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

[JENKINS-51658] add reincrementalify to completionGoals #117

Merged
merged 1 commit into from
Jul 16, 2018

Conversation

jetersen
Copy link
Member

@jetersen jetersen commented Jul 12, 2018

cc @jglick @oleg-nenashev

This could be applied to https://github.com/jenkinsci/pom as well 👍

Here we can see the completionGoals in use: jenkinsci/kotlin-v1-stdlib-jdk8-plugin@afc6746

@jetersen jetersen changed the title add completionGoals to reincrementalify add reincrementalify to completionGoals Jul 12, 2018
@ndeloof
Copy link

ndeloof commented Jul 13, 2018

will this extra goal be a no-op on plugins without increment extension ?

@jetersen
Copy link
Member Author

jetersen commented Jul 13, 2018

Indeed that is the case @ndeloof
The activation on the profile should ensure that at least the .mvn/maven.config and revision property is set before activating. Would love to check for changelist too but my knowledge and/or maven it self is very limited in that regard. https://issues.apache.org/jira/browse/MNG-3328

      <activation>
        <property>
          <name>revision</name>
        </property>
        <file>
          <exists>${basedir}/.mvn/maven.config</exists>
        </file>
      </activation>

@jetersen
Copy link
Member Author

jetersen commented Jul 13, 2018

Oh I can do one better 😃 I can just check for might-produce-incrementals property!

@oleg-nenashev
Copy link
Member

might != will though

@jetersen
Copy link
Member Author

jetersen commented Jul 13, 2018

No but if the profile is activate, it means when you release you still get the benefit of completion goals sadly maven is limited once again and you cannot chain profiles

@jetersen
Copy link
Member Author

jetersen commented Jul 13, 2018

I can't really see a good way of activating this profile without assuming you are using incremental if you have .mvn/config because of limitiation on maven.

if we make reincrementalify check if revision and changelist properties are in the POM before changing the pom we could make it fault tolerant and not break anybody.

@jetersen
Copy link
Member Author

jetersen commented Jul 13, 2018

Added jenkinsci/incrementals-tools#5 to avoid breaking developers POM if for some odd reason they have .mvn/config with no revision or changelist in their POM when they try to perform a release.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Too much work and unreliable. Just put this configuration in the might-produce-incrementals profile and it will be activated exactly when it is needed.

@jglick
Copy link
Member

jglick commented Jul 14, 2018

Yes something similar should go into jenkinsci/pom but beware that @kohsuke’s automated script for releasing jenkins already reincrementalify and commit and push, so it would probably break—we would need to coördinate with him to turn that off.

@batmat batmat self-requested a review July 14, 2018 12:22
@jetersen
Copy link
Member Author

Followed your advice @jglick 👍

@oleg-nenashev oleg-nenashev merged commit 4449643 into jenkinsci:master Jul 16, 2018
@jetersen jetersen deleted the supportCompletionGoals branch July 16, 2018 08:32
@jglick
Copy link
Member

jglick commented Jul 17, 2018

JENKINS-51658 FTR

@jglick jglick changed the title add reincrementalify to completionGoals [JENKINS-51658] add reincrementalify to completionGoals Jul 17, 2018
jglick added a commit to jglick/mrp-test that referenced this pull request Jul 17, 2018
@@ -1305,6 +1305,9 @@
</plugin>
</plugins>
</build>
<properties>
Copy link
Member

Choose a reason for hiding this comment

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

As noted in jenkinsci/pom#27 (review) this would better be done using mojo configuration.

@jetersen
Copy link
Member Author

I'll do a follow up 😊

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.

4 participants