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

Describe how to manually manage parts of the version number #4933

Merged
merged 6 commits into from
Mar 4, 2022

Conversation

daniel-beck
Copy link
Contributor

Many developers are surprised about the big version numbers, so

  1. Make that more explicit
  2. Offer an alternative

Because alternatives don't fit great into the current list format, I switched that to separate sections.

@daniel-beck daniel-beck requested a review from a team as a code owner February 18, 2022 12:00
@probot-autolabeler probot-autolabeler bot added the documentation Jenkins documentation, including user and developer docs, solution pages, etc. label Feb 18, 2022
@jglick
Copy link
Contributor

jglick commented Feb 18, 2022

alternatives don't fit great into the current list format

Nested bullets? 3a vs. 3b? Not sure.

<properties>
- <revision>1.2.3</revision>
- <changelist>-SNAPSHOT</changelist>
+ <revision>1.2</revision>
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think we should recommend this. It just does not play well with the concept of automated release.

It is fine to document using versions like 2.$generated (where the plugin was previously on 1.x) just so that you can change your mind and go back to MRP with 3.0 but if all goes well never touch the 2. part again (or ultimately drop it). But we should not advertise use of semantic versioning, which JEP-229 does not really support with a reasonable UX.

(You can use Conventional Commits to generate version numbers for you. But this will mean that the version number is not deterministically implied by the commit. That means that any tools which expect to check out the /scm/tag and locally build the same version, like PCT, will get confused. This is a reasonable thing to do for a service, but problematic for something like a Jenkins plugin which might be a Maven dependency of something else.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like I your concerns are addressed by lines 147 ("or you simply prefer") and 174-175? Or is this just about the "semantic versioning" choice of term?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO

For a component whose version number should reflect some semantic versioning, or you simply prefer to have more classic major version numbers (like Jenkins core), keep `<revision>` in the `<properties>` section, setting it to the prefix (`major`, `major.minor`, etc., depending on how much of the version number you want to manually manage) and use it as part of the top-level `<version>` element:
does not capture what I consider the legitimate use case—wishing to keep major numbers small enough so as to defer a binding decision about JEP-229; and
Ideally you always change the `revision` as part of the changes that semantically would require a `revision` change for the next release to prevent accidental automatic releases that do not use a new `revision`.
is advertising something that would in practice be too painful to seriously adopt, and which would largely negate the “CD” aspect of JEP-229.

If people want to keep the MRP model, and simply do not want to have local credentials, then jenkins-infra/jenkins-maven-cd-action#14 would make more sense.

(BTW prefer using one source code line per sentence or major clause, for easier editing, reviewing, and linking.)

Copy link
Contributor

@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.

I am afraid I find the diff here pretty much unreviewable due to all the structure changes. (Same when ignoring whitespace, or viewing rich diff.)

+
NOTE: It is worth communicating this to your users, as they will see a very different version number format than before.
The best way to do this is to add a line to the release notes: link:https://github.com/jenkinsci/azure-artifact-manager-plugin/releases/tag/86.va2aa4b1038c7[example note].
Here the version numbers will look like `1.321.vabcdef456789` or `1.999999-SNAPSHOT`, respectively, which are more similar to version numbers used by Jenkins itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean

Suggested change
Here the version numbers will look like `1.321.vabcdef456789` or `1.999999-SNAPSHOT`, respectively, which are more similar to version numbers used by Jenkins itself.
Here the version numbers will look like `1.321.vabcdef456789` or `1.999999-SNAPSHOT`, respectively, which are more similar to version numbers used by Jenkins core.

?

content/doc/developer/publishing/releasing-cd.adoc Outdated Show resolved Hide resolved
content/doc/developer/publishing/releasing-cd.adoc Outdated Show resolved Hide resolved
@daniel-beck
Copy link
Contributor Author

I am afraid I find the diff here pretty much unreviewable due to all the structure changes

We have preview deployments if you want to take a look at the live site, maybe that helps? https://deploy-preview-4933--jenkins-io-site-pr.netlify.app/doc/developer/publishing/releasing-cd/

Co-Authored-By: Jesse Glick <jglick@cloudbees.com>
@jglick
Copy link
Contributor

jglick commented Feb 23, 2022

We have preview deployments if you want to take a look at the live site

Overkill, it is easy to view the full page in AsciiDoc preview in GH. Just hard to see the diff.

Copy link
Contributor

@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.

Looks better. I would still recommend explicitly discussing the reasoning behind Manually controlled prefix.

If you do not want to have (major) version numbers increase quickly

and

more similar to version numbers used by Jenkins itself

do not really capture the (out of band) discussion, which was about people being leery of committing up front to having major version numbers be in the triple digits, with no option of going back to MRP-style versioning except by starting at say 1000.1, because version numbers going forward must be mathematically larger than any currently on the UC.

Comment on lines +151 to 170
--- a/.mvn/maven.config
+++ b/.mvn/maven.config
@@ -1,2 +1,3 @@
-Pconsume-incrementals
-Pmight-produce-incrementals
+-Dchangelist.format=%d.v%s
--- a/pom.xml
+++ b/pom.xml
@@ -10,12 +10,12 @@
<artifactId>some-library-wrapper</artifactId>
- <version>${revision}${changelist}</version>
+ <version>${revision}.${changelist}</version>
<packaging>hpi</packaging>
<properties>
- <revision>1.2.3</revision>
- <changelist>-SNAPSHOT</changelist>
+ <revision>1</revision>
+ <changelist>999999-SNAPSHOT</changelist>
<jenkins.version>2.176.4</jenkins.version>
----
Copy link
Contributor

@kuisathaverat kuisathaverat Mar 4, 2022

Choose a reason for hiding this comment

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

Why not directly a suggestion? I do not see the point of a diff, it could not match with the plugin's config. It is more clear and straigforward.

.mvn/maven.config

-Pconsume-incrementals
-Pmight-produce-incrementals
-Dchangelist.format=%d.v%s

pom.xml

   <artifactId>some-library-wrapper</artifactId>
   <version>${revision}.${changelist}</version> <!-- final version format `1.321.vabcdef456789` or `1.999999-SNAPSHOT` -->
...
   <properties>
     <revision>1</revision> <!-- whatever version you have as current major baseline -->
     <changelist>999999-SNAPSHOT</changelist> <!-- cahngelist format `321.vabcdef456789` or `999999-SNAPSHOT`-->
...

Copy link
Contributor Author

@daniel-beck daniel-beck Mar 4, 2022

Choose a reason for hiding this comment

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

Carelessly applied, that changes both your artifact ID and Jenkins baseline.

In particular, the diff seems useful e.g. with maven.config so people don't remove other, unrelated content they have in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, I give some credit to the Jenkins plugin maintainer to understanding the pom.xml and the maven.config files

Copy link
Contributor Author

@daniel-beck daniel-beck Mar 4, 2022

Choose a reason for hiding this comment

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

Explain how we have these CD-"enabled" plugins then, where maintainers evidently failed to follow these instructions:

console-column-plugin:2.0.1-rc59.8fe210d5e9b_3
jobConfigHistory:2.31-rc1107.2354f08725a_8
leanix-microservice-intelligence:1.0.11-rc201.335c08161b66
minio:1.3.3-rc93.9e92f846d4cf
purge-build-queue-plugin:2.0.0-rc11.9c20b_30c9572
remoting-opentelemetry:1.0-rc89.d67d14d05962
skip-cron-rebuild:1.1-rc20.50497e5a8a3a
talend:1.4-rc43.dbb2c0671f67

In my experience, many plugin maintainers only have very basic understanding of Maven, some don't even know Java well (according to themselves).

@jglick
Copy link
Contributor

jglick commented Mar 4, 2022

Conflicts with #4961. Need to move this forward. Shall I offer a smaller patch which does not reformat the whole file?

@daniel-beck
Copy link
Contributor Author

Just needs someone to actually approve this 🤷

@jglick
Copy link
Contributor

jglick commented Mar 4, 2022

#4933 (review) still, but better than status quo.

@jglick
Copy link
Contributor

jglick commented Mar 4, 2022

Another merge conflict with #4963. 🤷

@daniel-beck
Copy link
Contributor Author

That could as well have been a suggested change here…

@timja timja merged commit 91a7d6c into jenkins-infra:master Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Jenkins documentation, including user and developer docs, solution pages, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants