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

enable cd #484

Merged
merged 4 commits into from
Jan 28, 2022
Merged

enable cd #484

merged 4 commits into from
Jan 28, 2022

Conversation

car-roll
Copy link
Contributor

pom.xml Outdated
@@ -34,7 +34,7 @@
<modelVersion>4.0.0</modelVersion>
<groupId>org.jenkinsci.plugins</groupId>
<artifactId>pipeline-model-parent</artifactId>
<version>${revision}${changelist}</version>
<version>${revision}.${changelist}</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

per @jtnord 's suggestion, going with a 2.{revision}.{changelist}

Copy link
Member

Choose a reason for hiding this comment

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

Which suggestion? Why?

Copy link
Member

Choose a reason for hiding this comment

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

whilst you do not care about semver having a prefix instead of just starting with a huge number means we can more easily to compatable-since (can just bump the first number and say since 3.0 without having to constantly update it if something else gets merged first) and well these numbers are meaningless anyway so have a 2.whatever is no worse than otherwise, and allows us to have the compatable-since should it ever be needed (so is a good general practice in my opinion).

ie there is a pro with no noticeable drawback as far as I can see, as this gives us more flexibility to do things in the future.

Copy link
Member

Choose a reason for hiding this comment

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

No major drawback, just extra stuff in the version string before what you need to pay attention to, and inconsistency with other plugins.

In the rare case that a compatibleSince is added or updated, this is easily supported: leave that version TODO in the PR, and when merging, do so by hand and make the merge commit use $(git rev-list --count master) + 1.

Copy link
Member

Choose a reason for hiding this comment

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

this is easily supported

but requires a special flow - which I think is good to avoid in general?

Copy link
Member

Choose a reason for hiding this comment

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

If this is were regular occurrence perhaps. AFAICT here it is not frequent, especially recently (5bb4e37, #174, 10f7c8e, #393) so I would pick the cosmetically simpler version and deal with the extra few minutes’ work every couple of years. FWIW

@car-roll car-roll requested a review from a team January 26, 2022 19:13
@@ -1,2 +1,3 @@
-Pconsume-incrementals
-Pmight-produce-incrementals
-Dchangelist.format=%d.v%s
Copy link
Member

Choose a reason for hiding this comment

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

@jtnord
Copy link
Member

jtnord commented Jan 27, 2022

build failed as other properties need to be updated. or we just change the version to be changelist and change the changelist format to Dchangelist.format=2.%d.v%s which is probably simple for the multi module should we wish to keep the 2. prefix

@@ -1,3 +1,3 @@
-Pconsume-incrementals
-Pmight-produce-incrementals
-Dchangelist.format=%d.v%s
-Dchangelist.format=2.%d.v%s
Copy link
Member

Choose a reason for hiding this comment

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

Standard

Suggested change
-Dchangelist.format=2.%d.v%s
-Dchangelist.format=%d.v%s

suffices IMO.

@car-roll car-roll merged commit 8303080 into jenkinsci:master Jan 28, 2022
@car-roll car-roll deleted the enable-cd branch January 28, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants