Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APPSERV-59 Adds MicroProfile 2.3 profile #103

Merged
merged 4 commits into from
Apr 3, 2020
Merged

APPSERV-59 Adds MicroProfile 2.3 profile #103

merged 4 commits into from
Apr 3, 2020

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Mar 20, 2020

In connection with PR payara/Payara#4582.
That PR should be merged first.

@@ -145,7 +164,7 @@
<activation>
<property>
<name>payara.version</name>
<value>/^5\.193(\.\d*)?(-SNAPSHOT)?/</value>
<value>/5\.19[^1].*|5\.201.*/</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this now match against 5.192? Conflict with profile above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also 5.201 uses metrics 2.2 not 2.0.1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I did not think about the default and the version interaction. This needs another profile for 5.201

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out we supported metrics 2.2 from 5.194, so the new profile needs to match both 5.194 and 5.201, with 5.193 supporting metrics 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking that up

@jbee
Copy link
Contributor Author

jbee commented Apr 2, 2020

@Pandrex247 I think now I got the numbers right.

<activation>
<property>
<name>payara.version</name>
<value>/5\.194.*|5\.201.*/</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably retain the possibility of having -SNAPSHOT at the end due to stability stream / enterprise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the .* allow that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have thought so but there seems to be something wrong with the regex (which isn't obvious to me)

mvn help:active-profiles  -Dpayara.version=5.201-SNAPSHOT -am -pl MicroProfile-Metrics/tck-runner ---> mp-metrics-2.2

mvn help:active-profiles  -Dpayara.version=5.201.1-SNAPSHOT -am -pl MicroProfile-Metrics/tck-runner ---> Error (same as below - no property version found)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only assume this is due to the illegal regex provided in the other profile (missed / in the start). I tested this after that was corrected and it worked. Another issue was that the CDI api version 2.0.2 was incorrect.

<activation>
<property>
<name>payara.version</name>
<value>5\.20[^1].*/</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't match against 5.202-SNAPSHOT either, but gets by due to default property value.
It seems to outright fail though against 5.202.1-SNAPSHOT - weirdly because it seems to have lost the default value:

andrew@OpenSUSE:~/Git/MicroProfile-TCK-Runners> mvn help:active-profiles  -Dpayara.version=5.202.1-SNAPSHOT -Ppayara-server-remote -pl MicroProfile-Metrics/tck-runner
[INFO] Scanning for projects...
[ERROR] [ERROR] Some problems were encountered while processing the POMs:
[ERROR] 'dependencies.dependency.version' for org.eclipse.microprofile.rest.client:microprofile-rest-client-tck:jar must be a valid version but is '${microprofile.rest.client.tck.version}'. @ line 105, column 22
 @ 
[ERROR] The build could not read 1 project -> [Help 1]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is because 5\.20[^1].*/ is missing the / at the beginning.

@jbee
Copy link
Contributor Author

jbee commented Apr 3, 2020

@Pandrex247 Sorry I was bit sloppy with the testing myself. Now I tested all the possible version and it works for me so it should work for you. .* does cover any .1 or -SNAPHOT versions.

@Pandrex247
Copy link
Member

I'm still getting failures when I specify 5.201.1-SNAPSHOT or 5.202.1-SNAPSHOT.
Does it work for you? I'm not seeing anything wrong with the regex so I wonder if it's my env.

@jbee
Copy link
Contributor Author

jbee commented Apr 3, 2020

 mvn clean install "-Dpayara.version=5.201.1-SNAPSHOT"

gives me

     [echo] Metrics version     2.2.payara-p1
     [echo] Metrics TCK version 2.2.payara-p1

and

 mvn clean install "-Dpayara.version=5.201.1-SNAPSHOT"

gives me

     [echo] Metrics version     2.3
     [echo] Metrics TCK version 2.3

@Pandrex247
Copy link
Member

Ahhh, it doesn't seem to like it when I run it from the top-level and specify the project - works fine from the actual module directory

@jbee
Copy link
Contributor Author

jbee commented Apr 3, 2020

Ahhh, it doesn't seem to like it when I run it from the top-level and specify the project - works fine from the actual module directory

Yes, profiles and version match behave strange when not run that the leave project. I noticed this before. Not sure this is an issue with profiles in general or just with version matching plugin.

@jbee jbee merged commit 5cff11b into payara:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants