Skip to content
This repository has been archived by the owner on Oct 14, 2020. It is now read-only.

Pull new boosters fix #329

Merged
merged 3 commits into from
Aug 14, 2019
Merged

Conversation

awisniew90
Copy link
Member

@awisniew90 awisniew90 commented Aug 7, 2019

This PR includes and builds off of changes to PR #328. Additional changes are:

  1. Booster version fix
  2. XML formatting
  3. Use of aggregate api dependencies

Additional issues opened as a result of this PR:

  1. Update doc to fix matrix and test coverage (Doc: Update supported boosters #330 )
  2. Config constants refactoring (Refactor Config Constants class #331)
  3. Fix tests using hardcoded dependency versions (Remove "SNAPSHOT" from hardcoded booster versions in test classes #332)

Signed-off-by: Andrew J. Mauer <[email protected]>

remove booster dependencies from the api boms based on matt's comments

Signed-off-by: Andrew J. Mauer <[email protected]>

fix version numbers and related unit tests based on scott's review comment

Signed-off-by: Andrew J. Mauer <[email protected]>

add copyrights and fix comments based on scott's review comment

Signed-off-by: Andrew J. Mauer <[email protected]>

fix boms to use new booster version numbers

Signed-off-by: Andrew J. Mauer <[email protected]>

fixed missed update for jsonp version in the ee8 bom

Signed-off-by: Andrew J. Mauer <[email protected]>
@awisniew90 awisniew90 force-pushed the pullNewBoosters_fix branch 2 times, most recently from bdfcf8b to e3a259e Compare August 7, 2019 15:10
@scottkurz
Copy link
Contributor

Can't see why Travis failed since the inner Maven build seemed OK to me... restarting.

<scope>import</scope>
<type>pom</type>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should import runtime BOM like EE BOMs do.

<dependency>
<groupId>boost.boosters</groupId>
<artifactId>boost-ee8-apis-bom</artifactId>
<version>0.1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Versioned along with plugin

<dependency>
<groupId>boost.boosters</groupId>
<artifactId>boost-ee8-apis-bom</artifactId>
<version>0.1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Versioned with plugin.

@@ -38,26 +31,11 @@

<dependencyManagement>
Copy link
Contributor

Choose a reason for hiding this comment

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

For MP API boms, we should just import existing Eclipse BOMs from project: https://github.com/eclipse/microprofile/blob/master/pom.xml. A couple seem to also define EE API versions, which means we should start by NOT importing EE API BOM, until we realize we have a reason to do so.

<dependencyManagement>
<dependencies>
<dependency>
<groupId>boost.boosters</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment about reusing Eclipse BOMs... but if we were to use our EE API BOMs, we probably need to use an import here (unless I'm missing the purpose).

<artifactId>jaxrs</artifactId>
<version>2.1.M1-SNAPSHOT</version>

<dependencyManagement>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just not import this since we keep getting confused about how the build hangs together. Either try to use a property at the parent POM level for the JAXRS API version (and access both from the API bom and here) so we don't repeat the prop value, or just copy-paste (and still use a simlar-named property here).


<groupId>boost.boosters</groupId>
<artifactId>cdi</artifactId>
<version>2.0.M1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add an extra digit for boost-specific changes.

So it'd be:
<spec-Major>.<spec-Minor>.<rev-for-this-specific-booster>.<milestone>-?SNAPSHOT?

<version>0.1-SNAPSHOT</version>
<groupId>boost.runtimes</groupId>
<artifactId>openliberty</artifactId>
<version>0.1-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

The 0.1-SNAPSHOT servers the purpose of separtely versioning from the pluging..but maybe 1.0-M1-SNAPSHOT would be better?

@scottkurz scottkurz merged commit a160dbb into MicroShed:master Aug 14, 2019
@scottkurz
Copy link
Contributor

Merged in #328 so closing this. I think we captured all follow-ups needed during the review here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants