[GitHub] [tomee] cocorossello opened a new pull request #779: Fix some examples using BOM

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] cocorossello opened a new pull request #779: Fix some examples using BOM

GitBox

cocorossello opened a new pull request #779:
URL: https://github.com/apache/tomee/pull/779


   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] cocorossello commented on pull request #779: Fix some examples using BOM

GitBox

cocorossello commented on pull request #779:
URL: https://github.com/apache/tomee/pull/779#issuecomment-812988072


   Still not working:
   examples/polling-parent/polling-web/pom.xml
   examples/applicationcomposer-jaxws-cdi/pom.xml


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #779:
URL: https://github.com/apache/tomee/pull/779#discussion_r607018168



##########
File path: examples/polling-parent/pom.xml
##########
@@ -79,29 +79,17 @@
     <dependencies>
       <!-- API -->
       <dependency>
-        <groupId>org.apache.tomee</groupId>
-        <artifactId>javaee-api</artifactId>
-        <version>[8.0,)</version>
-        <scope>provided</scope>
-      </dependency>
-      <dependency>
-        <groupId>org.apache.tomee</groupId>
-        <artifactId>openejb-api</artifactId>
-        <version>${tomee.version}</version>
+        <groupId>org.apache.tomee.bom</groupId>
+        <artifactId>tomee-plus-api</artifactId>
+        <version>8.0.7-SNAPSHOT</version>
         <scope>provided</scope>
       </dependency>
 
       <!-- impl - for standard code scope test is fine -->
       <dependency>
-        <groupId>org.apache.tomee</groupId>
-        <artifactId>openejb-core</artifactId>
-        <version>${tomee.version}</version>
-        <scope>provided</scope>
-      </dependency>
-      <dependency>
-        <groupId>org.apache.tomee</groupId>
-        <artifactId>openejb-cxf-rs</artifactId>
-        <version>${tomee.version}</version>
+        <groupId>org.apache.tomee.bom</groupId>
+        <artifactId>tomee-plus-api</artifactId>

Review comment:
       Should probably be `tomee-plus` instead (otherwise duplicate). Would also go for scope `test` here (as `test` would be finde for standard code but we might need to override it in submodules)

##########
File path: examples/polling-parent/polling-core/pom.xml
##########
@@ -29,20 +29,18 @@
 
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>openejb-api</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>
     </dependency>
     <dependency>
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>
     </dependency>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>openejb-core</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus</artifactId>
+      <version>8.0.7-SNAPSHOT</version>

Review comment:
       No version needed. Tree gets it for free from the parent dep mgmt section.
   We have some dependencies towards `openjpa` here, so we would need `provided` here (only we have `test` in the parent pom)

##########
File path: examples/polling-parent/polling-client/pom.xml
##########
@@ -30,27 +30,9 @@
 
   <dependencies>
     <dependency>
-      <groupId>org.apache.cxf</groupId>
-      <artifactId>cxf-core</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>org.apache.cxf</groupId>
-      <artifactId>cxf-rt-rs-client</artifactId>
-      <version>${cxf.version}</version>
-      <exclusions>
-        <exclusion>
-          <groupId>javax.annotation</groupId>
-          <artifactId>javax.annotation-api</artifactId>
-        </exclusion>
-      </exclusions>
-    </dependency>
-    <dependency>
-      <groupId>org.apache.xbean</groupId>
-      <artifactId>xbean-finder-shaded</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>org.apache.xbean</groupId>
-      <artifactId>xbean-reflect</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus</artifactId>
+      <version>8.0.7-SNAPSHOT</version>

Review comment:
       No version needed. Tree gets it for free from the parent dep mgmt section.
   If you go for `test` in the parent, we need to set it to `provided` here as we have some dependencies.

##########
File path: examples/polling-parent/polling-web/pom.xml
##########
@@ -35,21 +35,14 @@
       <artifactId>junit</artifactId>
     </dependency>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>openejb-core</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>openejb-cxf-rs</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus</artifactId>
+      <version>8.0.7-SNAPSHOT</version>

Review comment:
       No version needed. Tree gets it for free from the parent dep mgmt section.

##########
File path: examples/polling-parent/pom.xml
##########
@@ -79,29 +79,17 @@
     <dependencies>
       <!-- API -->
       <dependency>
-        <groupId>org.apache.tomee</groupId>
-        <artifactId>javaee-api</artifactId>
-        <version>[8.0,)</version>
-        <scope>provided</scope>
-      </dependency>
-      <dependency>
-        <groupId>org.apache.tomee</groupId>
-        <artifactId>openejb-api</artifactId>
-        <version>${tomee.version}</version>
+        <groupId>org.apache.tomee.bom</groupId>
+        <artifactId>tomee-plus-api</artifactId>
+        <version>8.0.7-SNAPSHOT</version>

Review comment:
       Maybe use `tomee.version` instead of the hard string as it is already defined in the `properties` section. We also can remove `cxf.version` and `xbean.version` from the `<properties` section of the parent pom.
   
   In addition, we can remove
   
   ```
    <dependency>
           <groupId>org.apache.cxf</groupId>
           <artifactId>cxf-core</artifactId>
           <version>${cxf.version}</version>
         </dependency>
         <dependency>
           <groupId>org.apache.xbean</groupId>
           <artifactId>xbean-finder-shaded</artifactId>
           <version>${xbean.version}</version>
         </dependency>
         <dependency>
           <groupId>org.apache.xbean</groupId>
           <artifactId>xbean-reflect</artifactId>
           <version>${xbean.version}</version>
         </dependency>
   ```
   
   as it comes with the related BOM.

##########
File path: examples/polling-parent/polling-core/pom.xml
##########
@@ -29,20 +29,18 @@
 
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>openejb-api</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>

Review comment:
       No version needed. Tree gets it for free from the parent dep mgmt section.

##########
File path: examples/applicationcomposer-jaxws-cdi/pom.xml
##########
@@ -64,10 +64,10 @@
   </repositories>
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-      <version>[8.0,)</version>
-      <scope>provided</scope>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>
+      <scope>test</scope>

Review comment:
       Shouldn't it be `provided` ?

##########
File path: examples/polling-parent/polling-client/pom.xml
##########
@@ -62,8 +44,9 @@
       <version>${project.version}</version>
     </dependency>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>

Review comment:
       No version needed. Tree gets it for free from the parent dep mgmt section.

##########
File path: examples/polling-parent/polling-domain/pom.xml
##########
@@ -29,8 +29,9 @@
 
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>

Review comment:
       No version needed. Tree gets it for free from the parent dep mgmt section.

##########
File path: examples/applicationcomposer-jaxws-cdi/pom.xml
##########
@@ -64,10 +64,10 @@
   </repositories>
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-      <version>[8.0,)</version>
-      <scope>provided</scope>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>
+      <scope>test</scope>

Review comment:
       Otherwise:
   
   It seems, that the tests are failing as the `tomee-plus` boms brings in `tomee-security`, which expects some servlet registrations in `TomEESecurityServletAuthenticationMechanismMapper` and subsequently fails with a `NullPointerException`.
   
   So either
   
   - (a) The test is flawed and requires some additional parameters / registrations
   - (b) `TomEESecurityServletAuthenticationMechanismMapper` requires a ` != null` check on `servletRegistrations` in `init(...)`
   - (c) Exclude `tomee-security` in the examples dependencies
   - (d) Something else (disable security in the example, other things, ...) ....
   
   Maybe @dblevins has some thoughts here?

##########
File path: examples/polling-parent/polling-web/pom.xml
##########
@@ -35,21 +35,14 @@
       <artifactId>junit</artifactId>
     </dependency>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>openejb-core</artifactId>
-    </dependency>
-    <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>openejb-cxf-rs</artifactId>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus</artifactId>
+      <version>8.0.7-SNAPSHOT</version>

Review comment:
       In addition: `provided` if `test` in parent pom.




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] dblevins commented on a change in pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

dblevins commented on a change in pull request #779:
URL: https://github.com/apache/tomee/pull/779#discussion_r607192663



##########
File path: examples/applicationcomposer-jaxws-cdi/pom.xml
##########
@@ -64,10 +64,10 @@
   </repositories>
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-      <version>[8.0,)</version>
-      <scope>provided</scope>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>
+      <scope>test</scope>

Review comment:
       My recommendation would be to for both C and B.  Specifically, do the exclude (B) so we can get the build working, then execute C and remove the exclude.




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] dblevins commented on a change in pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

dblevins commented on a change in pull request #779:
URL: https://github.com/apache/tomee/pull/779#discussion_r607192663



##########
File path: examples/applicationcomposer-jaxws-cdi/pom.xml
##########
@@ -64,10 +64,10 @@
   </repositories>
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-      <version>[8.0,)</version>
-      <scope>provided</scope>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>
+      <scope>test</scope>

Review comment:
       My recommendation would be to for both C and B.  Specifically, do the exclude (B) in this PR so we can get the build working, then execute C in a different PR and remove the exclude.




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] cocorossello commented on a change in pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

cocorossello commented on a change in pull request #779:
URL: https://github.com/apache/tomee/pull/779#discussion_r607245206



##########
File path: examples/applicationcomposer-jaxws-cdi/pom.xml
##########
@@ -64,10 +64,10 @@
   </repositories>
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-      <version>[8.0,)</version>
-      <scope>provided</scope>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>
+      <scope>test</scope>

Review comment:
       Excluded, thanks, I didn't know whether servletRegistrations should be not null or (more likely) servletRegistrations should not be called




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on a change in pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

rzo1 commented on a change in pull request #779:
URL: https://github.com/apache/tomee/pull/779#discussion_r607288878



##########
File path: examples/applicationcomposer-jaxws-cdi/pom.xml
##########
@@ -64,10 +64,10 @@
   </repositories>
   <dependencies>
     <dependency>
-      <groupId>org.apache.tomee</groupId>
-      <artifactId>javaee-api</artifactId>
-      <version>[8.0,)</version>
-      <scope>provided</scope>
+      <groupId>org.apache.tomee.bom</groupId>
+      <artifactId>tomee-plus-api</artifactId>
+      <version>8.0.7-SNAPSHOT</version>
+      <scope>test</scope>

Review comment:
       @cocorossello The `api` module needs to be in scope `provided` in this example. Besides that: Works fine for me locally :) (and all other examples are passing the tests too... so overall lgtm. thx!)
   
   




--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] dblevins commented on pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

dblevins commented on pull request #779:
URL: https://github.com/apache/tomee/pull/779#issuecomment-814559280


   Not sure why Jenkins didn't build this PR after it was updated, but merging to see if we can fix the build.
   
   Thank you @cocorossello


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] dblevins merged pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

dblevins merged pull request #779:
URL: https://github.com/apache/tomee/pull/779


   


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomee] rzo1 commented on pull request #779: Fix some examples using BOM

GitBox
In reply to this post by GitBox

rzo1 commented on pull request #779:
URL: https://github.com/apache/tomee/pull/779#issuecomment-814626199


   I opened a PR to test the integration a few days ago, which worked. @cocorossello uses a shared repository for the PRs - might be the reason why Jenkins does not pick up these PRs - would be imho an odd behaviour of the Jenkins Plugin.


--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]