Conversation
d18b867 to
3ab7d58
Compare
| </description> | ||
| <url>https://github.com/datastax/java-driver</url> | ||
|
|
||
| <url>https://github.com/datastax/java-driver/driver-core</url> |
There was a problem hiding this comment.
This link looks to be broken (should be: https://github.com/datastax/java-driver/tree/3.x/driver-core although would be nice if we could avoid having the branch there)
There was a problem hiding this comment.
Ah right, silly me :) I guess there's nothing to do other than simply remove the URL tag altogether, as it does not seem that there is a way to create a link without specifying a branch.
There was a problem hiding this comment.
👍 on removing the URL tag, for individual modules the parent link is probably appropriate anyways.
| <test.groups>unit</test.groups> | ||
| <jdk8.excludes>none</jdk8.excludes> | ||
| </properties> | ||
| <url>https://github.com/datastax/java-driver/driver-extras</url> |
There was a problem hiding this comment.
similarly to the core pom, this link will not work.
| <properties> | ||
| <groovy.version>2.4.7</groovy.version> | ||
| </properties> | ||
| <url>https://github.com/datastax/java-driver/driver-mapping</url> |
There was a problem hiding this comment.
similar to core, link broken
| <plugin> | ||
| <groupId>org.codehaus.gmaven</groupId> | ||
| <artifactId>gmaven-plugin</artifactId> | ||
| <version>1.5</version> |
There was a problem hiding this comment.
Was this just ultimately not needed? I see MapperGroovyTest is still running so things are still working on that front.
There was a problem hiding this comment.
Hmm I tried removing the plugin, but the groovy test did not execute.
| <test.skip>true</test.skip> | ||
| </properties> | ||
|
|
||
| <url>https://github.com/datastax/java-driver/driver-tests/osgi</url> |
| <!-- do not upgrade until https://issues.apache.org/jira/browse/SUREFIRE-1302 is fixed --> | ||
| <version>2.18</version> | ||
| <configuration> | ||
| <properties> |
There was a problem hiding this comment.
This was done for #730, is it no longer needed with 2.18 or is there some other change that makes it no longer needed?
There was a problem hiding this comment.
It is still needed, but the configuration is inherited from the main plugin declaration under section pluginManagement.
There was a problem hiding this comment.
ah! I didn't catch that, I ran a profile and didn't see it open / close many files in short succession, so it is still doing what it's supposed to.
|
It is really odd to me how |
|
It looks like tests are not running in class order, or maybe they are running concurrently which is causing weird behavior. Looking at the surefire config to see if it's possible that we're forking. |
|
Example of what i was referring to about test order: I don't seem to experience the same thing when running locally, so not quite sure what can be happening yet. |
|
Created #853 to fix the failing tests. |
| install: | ||
| - ps: .\ci\appveyor.ps1 | ||
| build_script: | ||
| - "set \"JAVA_HOME=%JAVA_8_HOME%\" && mvn install -DskipTests=true -D\"maven.javadoc.skip\"=true -B -V" |
There was a problem hiding this comment.
Not needed anymore, javadocs are only generated if the release profile is activated.
|
Squashed commits in anticipation of feature complete today. |
|
It looks like there may be a few inconsistent test issues caused by ordering changes, but I can take care of those on a different branch 👍 . There's still a few more things i need to try, but this looks good so far. The only thing I'm not sure about is the maven release plugin changes since it's not something I'm very familiar with, but I think I can use our artifactory deploy job to verify that this works so will give that a shot today. (Note that if it takes a while for me to get to that, I think this is in a good enough state to merge as we can always work out release plugin issues as part of the release) |
You can totally try out a release by issuing the following commands: The first command will create tags in Github and you'll have to delete them manually (I suggest using a lower release number just in case, e.g. 0.0.1).
That would be awesome. I did everything bearing in mind that a regular deploy to our internal Artifactory should work just as before. |
Nice call. I'll try the prepare part on a personal fork (just need to change the repo references and such). The artifactory deploy worked by the way so that checks out. |
|
Question, I just noticed that the licenses/developers/scm sections in pom.xml are repeated in all the child modules. If we remove those from the child modules can they be inherited from the parent? That could reduce some more duplication as well. |
|
Experimenting with the release plugin on my personal fork seemed to work (tag was appropriately created and my target branch was updated with the appropriate commits for incrementing versions in pom.xml). 👍 |
tolbertam
left a comment
There was a problem hiding this comment.
looks great to me! I had a small comment about duplication of licenses/scm/developer sections in the child modules, but that can be visited later (unless you wanted to approach that now). 👍 for merge.
I hesitated myself about it. Is it legally proved that the license stuff is inherited from the parent pom? |
I just tried it out and it seems to work. I deleted those sections from driver-core/pom.xml and ran in the driver-core directory: mvn help:effective-pomand those sections appeared to be inherited from the parent pom. |
|
Great, thanks for trying it out! I just removed these sections from all child poms. |
|
Looks great! Thanks for the quick change 👍 |
|
Did we try creating a staging repository on Sonatype OSS with the latest changes? Validations rules might be the reason why some sections were repeated in child poms. |
olim7t
left a comment
There was a problem hiding this comment.
Did not review everything in detail, but the changes for 1266 look ok to me.
Once again, may I kindly request that we keep commits (and ideally pull requests as well) more targeted? We're heading towards a single commit that will aggregate 1266, 1411 + additional changes, and purely cosmetic changes (e.g. blank lines). This is not cool for reviewers, mergers, or future maintainers looking at the history.
| <plugin> | ||
| <groupId>org.sonatype.plugins</groupId> | ||
| <artifactId>nexus-staging-maven-plugin</artifactId> | ||
| <version>1.6.7</version> |
There was a problem hiding this comment.
There's an 1.6.8 version in Maven central.
It looks like the changes are minor though, the main one is NEXUS-10502 which relates to proxy settings.
There was a problem hiding this comment.
Version bumped, thanks.
| <plugin> | ||
| <groupId>org.sonatype.plugins</groupId> | ||
| <artifactId>nexus-staging-maven-plugin</artifactId> | ||
| <version>1.6.7</version> |
There was a problem hiding this comment.
No need to repeat the version here if it's in pluginManagement.
There was a problem hiding this comment.
It seems that versions of plugins declared in a profile do not get inherited from plugins declared in pluginManagement. Here is the output of mvn versions:display-plugin-updates for driver-examples if I remove the version of this plugin declaration:
[WARNING] The following plugins do not have their version specified:
[WARNING] org.sonatype.plugins:nexus-staging-maven-plugin ............ 1.6.7
This might be a bug in the display-plugin-updates Mojo though, but I'd rather keep the version just in case.
There was a problem hiding this comment.
In hindsight, the build seems to work normally without specifying versions for plugins declared in profiles, so I went ahead and implemented your suggestion.
Removing OSS parent changes the configuration and version of several plugins (compiler, surefire, javadoc, install, deploy...). That's why it seemed simpler to do both 1511+1266 in one single commit. |
Yes, I validated that a staging repository is created and is valid wrt to OSS Sonatype rules (gpg signatures etc.).
Which ones? If you are referring to the nexus-staging plugin, indeed it must be mentioned a couple of times in child modules to prevent it from deploying unwanted artifacts. |
|
Maven central requires requires license, developer and scm sections for POMs, and my thought was that maybe it didn't take into account inheritance. But that was just a wild guess, if that's been validated then all good 👍 |
GregBestland
left a comment
There was a problem hiding this comment.
My Pom foo is weak, but after much deciphering I think this is gtg.
…cross the build Summary of changes: JAVA-1266: - Removed reference to deprecated OSS Sonatype parent poms - Adjusted release process to use the recommended method, as described in http://central.sonatype.org/pages/apache-maven.html - Created new 'release' profile to contain release-specific plugins JAVA-1411: - Introduced dependencyManagement and pluginManagement sections in the parent pom - Declared versions for all dependencies and plugins at parent pom - Concentrated common plugin configuration items into the parent pom - Revisited bundle and shade plugin configurations - Updated <url> tags - Removed unused 'env' system property - Profile declaration cleanup
Summary of changes:
JAVA-1266:
as described in http://central.sonatype.org/pages/apache-maven.html
JAVA-1411: