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

Support maven-release-plugin #14

Closed
wants to merge 9 commits into from
Closed

Conversation

jglick
Copy link
Contributor

@jglick jglick commented Dec 2, 2021

gh api -F ref=refs/tags/$version -F sha=$GITHUB_SHA /repos/$GITHUB_REPOSITORY/git/refs
if fgrep -sq changelist.format .mvn/maven.config
then # JEP-229
mvn -B -V -s $GITHUB_ACTION_PATH/settings.xml -ntp -Dstyle.color=always -Dset.changelist -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/releases/ -Pquick-build -P\!consume-incrementals clean deploy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to make this a configuration option so that people who missed incrementals don’t accidentally go this route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

people who missed incrementals

Or did incrementalify (e.g. via archetypes), but did not set up changelist.format. Yeah that is probably wise.

@jglick
Copy link
Contributor Author

jglick commented Dec 2, 2021

One important restriction: as per https://github.community/t/how-to-push-to-protected-branches-in-a-github-action/16101?u=jglick you must disable branch protection on your default branch, so e.g. Require status checks to pass before merging cannot be used.

jglick added a commit to jglick/plugin-pom that referenced this pull request Dec 2, 2021
@jglick jglick marked this pull request as ready for review December 3, 2021 00:05
@jglick jglick requested a review from timja December 3, 2021 00:06
@jglick jglick added the enhancement New feature or request label Dec 3, 2021
@jglick jglick changed the title Support MRP Support maven-release-plugin Dec 3, 2021
gh api -F ref=refs/tags/$version -F sha=$GITHUB_SHA /repos/$GITHUB_REPOSITORY/git/refs
if fgrep -sq changelist.format .mvn/maven.config
then # JEP-229
mvn -B -V -s $GITHUB_ACTION_PATH/settings.xml -ntp -Dstyle.color=always -Dset.changelist -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/releases/ -Pquick-build -P\!consume-incrementals clean deploy
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to make this a configuration option so that people who missed incrementals don’t accidentally go this route?

@jglick jglick marked this pull request as draft December 3, 2021 15:21
@justusbunsi
Copy link

One important restriction: as per https://github.community/t/how-to-push-to-protected-branches-in-a-github-action/16101?u=jglick you must disable branch protection on your default branch, so e.g. Require status checks to pass before merging cannot be used.

🤔 Instead of pushing directly to the default branch, a PR could be created. That way the branch protection can remain as-is.

@jglick
Copy link
Contributor Author

jglick commented Dec 6, 2021

Instead of pushing directly to the default branch, a PR could be created.

There is a Marketplace action that does something similar to work around this problem. In principle it could be possible to do that with https://maven.apache.org/maven-release/maven-release-plugin/prepare-mojo.html#pushChanges but it would make this action much more complicated—and even more error-prone than MRP usually is: you would be creating and tagging a release commit and publishing a release but the commit is still in a branch? What if that PR build flakes out and does not get automerged? And then another release build gets run? Etc.

If you care about branch protection, just do not use this mode. I do not recommend it to begin with and only developed it because I am still hesitant to move away from MRP on parent POMs specifically, despite the many drawbacks of MRP. Full JEP-229 is more straightforward: if everything goes well then the existing trunk commit is tagged as a release and that is that. (You can do the same just picking some arbitrary version string, like a timestamp or whatever, which would be fine enough for something like a WAR that is consumed solely as a binary artifact, but this does not work well for things like Jenkins components including plugins that might ever become <dependency>s of some other project.)

@dduportal
Copy link
Contributor

For what it's worth it, GitHub also allows the concept of "prerelease". We are using it on updatecli/updatecli , where we publish a release as "pre-release": it triggers our builds that generates the binaries. Last steps of these "prerelease" build is to set the release from "pre-release" as "latest release". Not sure how much this pattern could help there?

@jglick
Copy link
Contributor Author

jglick commented Dec 6, 2021

GitHub also allows the concept of "prerelease"

Yes; orthogonal I think. Discussion

@jtnord
Copy link

jtnord commented Dec 14, 2021

We can do releases with mrp without requiring to disable protected branches.
Push changes false and use something other than the auto generated version - which can be done by a simple plugin or extension or even bash script.

git config --global user.email [email protected]
git config --global user.name jenkins-maven-cd-action
git config --global url.https://github.com/.insteadOf [email protected]:
mvn -B -V -s $GITHUB_ACTION_PATH/settings.xml -ntp -Dstyle.color=always -P\!consume-incrementals -Darguments='-Pquick-build -ntp' validate release:prepare release:perform
Copy link

Choose a reason for hiding this comment

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

Of you are running a quick build you should probably just run validate as the preparation goal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The preparation goals verify that the artifact can actually be packaged. We also run validate first just to enforce the -P!consume-incrementals, which would not work inside preparation goals because of profile mayhem; jenkinsci/plugin-pom#416 would also catch mistakes but only after junk commits & tag were pushed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the action would have issues with branch protection 😢

Don't see this ever working unless you go around each repository and allow GitHub action bot to push commits past branch protections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Maybe this gha could be used to try to pass over branch protections?
https://github.com/marketplace/actions/branch-protection-bot

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although the GitHub action only removes include administrators, it does not remove branch protection entirely with status checks.

I believe you could create an GitHub action to retrieve current branch protect.
Remove the branch protection and restore it using an GitHub action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC https://github.com/marketplace/actions/branch-protection-bot#access_token cannot use $GITHUB_TOKEN, which I think makes that a non-starter.

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 just read through https://github.community/t/how-to-push-to-protected-branches-in-a-github-action/16101?u=jglick and https://github.community/t/allowing-github-actions-bot-to-push-to-protected-branch/16536?u=jglick and there really does not appear to be any satisfactory solution currently: if you want to use MRP, you must either give up on having protected branches (bad) or use a bot’s PAT (worse).

@jglick
Copy link
Contributor Author

jglick commented Dec 14, 2021

use something other than the auto generated version

Exactly what I do not want to happen. #14 (comment)

@jtnord
Copy link

jtnord commented Dec 16, 2021

use something other than the auto generated version

Exactly what I do not want to happen. #14 (comment)

The comment there is unclear as to why?

action much more complicated—and even more error-prone than MRP usually is: you would be creating and tagging a release commit and publishing a release but the commit is still in a branch? What if that PR build flakes out and does not get automerged? And then another release build gets run? Etc.

but we are conflating - you are not running m-r-p on PR builds - and if you are you most certainly will not be using the autogenerated versions IIUC here?

You do not need to push back the commits made by m-r-p to any branch at all - you just need to push the tag. if the release build fails who cares, try it again via the actions UI.

now if the tag is created but the build fails - again who cares - you have exactly this failire mode in what you are trying to do right now.

if you tell maven what version number to release as (multiple ways - but strip the -SNAPSHOT and add what the incrementals changenumber would be you can get 1.2.13005-ae00ddfc where 13005-ae00ddfc would be the same thing as the change.set generated for incrementals. and the version in the pom prior to release would be 1.2-SHAPSHOT

@jglick
Copy link
Contributor Author

jglick commented Dec 16, 2021

The comment there is unclear as to why?

Because of tools like PCT which expect the version loaded/built from a checkout of a tag to match the version actually deployed from that tag. Irrelevant for stuff like microservices where Maven is just a tool used to upload a binary in one direction, but relevant for projects like Jenkins which make heavy use of inter-repository artifact dependencies. MRP in default usage satisfies this constraint via its first junk commit (the tagged one fixing a non-SNAPSHOT version); as does JEP-229 (so long as -Dset.changelist is assumed).

You do not need to push back the commits made by m-r-p to any branch at all - you just need to push the tag.

If you are using MRP solely as a way to deploy and tag, which you can do more easily and safely without this plugin. If you want to use MRP in its normal sense, letting it manage versions of the component, then you need to push the junk commits as well as the tag, as this PR does.

the version in the pom prior to release would be 1.2-SHAPSHOT

And therein lies the problem, because that will be the POM version after the release as well, so you would just be releasing lots of 1.2.xxx versions until you manually edited the POM to be 1.3-SNAPSHOT. So you might as well just do JEP-229 and not deal with version numbers again. Which is the general recommendation.

Maybe I need to back up and explain the purpose of this PR. It is to allow a Maven component currently released via MRP on a developer machine in the typical way, e.g.

git checkout master
git pull --fast-forward
mvn release:{prepare,perform}
git pull --fast-forward

to be releasable from hosted infrastructure, optionally upon every significant PR merge, with no personal Artifactory credentials (solely GitHub write permission), and no need to manually publish draft release notes; but otherwise make no changes whatsoever to process, POM structure, version formats, etc., and retaining all of the other drawbacks of MRP in its normal usage mode—junk commits, race conditions, possibility of broken half-deployed states, etc.

What if that PR build flakes out and does not get automerged? And then another release build gets run?

but we are conflating - you are not running m-r-p on PR builds

Of course not. For this you need to look back over context. I was discussing a possible workaround to the problem of using MRP with branch protection enabled, given the limitation that GHA currently defines $GITHUB_TOKEN to have write but not administer permission sufficient to override branch protection. The notion was for the MRP script to disable the push of junk commits to master, and instead push them to a branch, file a PR for them, mark it for automerge if possible, and pray that it gets merged before another release is triggered and the versions get all messed up.

@jetersen
Copy link
Collaborator

jetersen commented Jul 13, 2022

I am still hesitant to move away from MRP on parent POMs specifically

Any particular reason?
Branch protection has benefits with dependency pull requests and build time, which plugin pom has a few dependencies :)

There have been several releases on plugin POM classified as breaking without bumping major version.

@jglick
Copy link
Contributor Author

jglick commented Jul 13, 2022

I am still hesitant to move away from MRP on parent POMs specifically

Any particular reason?

That it would require additional testing to be sure that the Flatten plugin is not going to mess something up. The JEP-229 version scheme seems to work fine for jenkinsci/bom which uses pom-packaging, but that is only imported. For a parent POM like jenkinsci/plugin-pom or jenkinsci/pom you need to make sure that the flattening is being applied only to the parent itself and not being inherited by child POMs. You also need to verify that all of the stuff which is supposed to be inherited by child POMs still will be (see https://www.mojohaus.org/flatten-maven-plugin/flatten-mojo.html#flattenMode). Certainly should be doable, I have just never gotten around to validating it, so I was trying this as what seemed like a lower-risk approach that at least retained the basic benefit of CD. (And might also be desired by some plugin authors, though I suspect that most of those seeking server-based releases yet uncomfortable with JEP-229 versioning would want #19 instead of MRP.)

@jetersen
Copy link
Collaborator

jetersen commented Jul 13, 2022

@jglick this might be the closest we get, this seems like a decent compromise, MBR is less noisy and PR is sent for version bump.

https://github.com/jenkinsci/gitea-plugin/blob/master/.github/workflows/cd.yaml

All doable with a new reusable workflow in the https://github.com/jenkins-infra/github-reusable-workflows

@jetersen
Copy link
Collaborator

jetersen commented Jul 13, 2022

@justusbunsi I do not see you actually uploading plugin during your CD flow, so are you manually running MBR locally? 😰

@jglick
Copy link
Contributor Author

jglick commented Jul 13, 2022

PR is sent for version bump

Yes this is possible. As I mentioned in #14 (comment) this seems (IMO) at least as problematic as the other alternatives. If you want to go ahead with that approach nonetheless, and set expectations very low, I will just close this PR. I was inclined to do so anyway.

Anyway I do not think justusbunsi@0855b92 suffices. If are you doing something like MRP but not actually using MRP, you need to recreate some of the stuff that MRP does in its release profile. JEP-305 adds that back in using the -Pproduce-incrementals profile. More importantly, you really need the pair of junk commits: one to set a non-snapshot version (and a proper /project/scm/tag), one to undo all that. https://github.com/jenkinsci/gitea-plugin/blob/1.4.3/pom.xml#L35 looks incorrect, so e.g. PCT would be broken. The *.hpi from the UC looks like it got the version right (though I do not follow how).

@justusbunsi
Copy link

justusbunsi commented Jul 13, 2022

@justusbunsi I do not see you actually uploading plugin during your CD flow, so are you manually running MBR locally? 😰

I think it's actually uploaded by the mvn command. https://github.com/justusbunsi/jenkins-maven-cd-action/blob/cc144a67ddc6ea116e038a99f1cb76c2cf07554f/run.sh#L15
There is no manual upload involved, so it should be that line. But I don't know how, tbh. 😅

EDIT: Looking into the last release build, this command was executed:

mvn -B -V -s /home/runner/work/_actions/justusbunsi/jenkins-maven-cd-action/v1.2.1/settings.xml -ntp -Dstyle.color=always -Dchangelist= -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/releases/ -Pquick-build '-P!consume-incrementals' clean deploy

Looks like the altDeploymentRepository combined with maven token does the upload?

@jetersen
Copy link
Collaborator

its the -DaltDeploymentRepository=maven.jenkins-ci.org::default::https://repo.jenkins-ci.org/releases/ combined with deploy that does it :)

@justusbunsi
Copy link

justusbunsi commented Jul 13, 2022

@jglick

If are you doing something like MRP but not actually using MRP,...

I think that fits the most/best. Especially for the Gitea plugin the other maintainer and I have decided that we wanted to keep readable versions but automate the releases workflow. So we might not follow the full MRP procedure right now. 😅

EDIT:

The *.hpi from the UC looks like it got the version right (though I do not follow how).

IIRC, it's due to the empty changeset during build. -Dchangeset= which removes the snapshot suffix?

@jetersen
Copy link
Collaborator

jetersen commented Jul 13, 2022

Could we achieve the same as MRP with out using MRP and no junk commits? Since it seems possible for JEP 229 use case to produce a POM that PCT can understand.
I simply don't understand how it can work in one use case but not the other.

scmTag and changeset can be updated via properties.

Is it because PCT require a POM without properties or flatten values.
Well it should be able to get that from the maven repository. Why does it need the same for git?

@jglick
Copy link
Contributor Author

jglick commented Jul 13, 2022

-Dchangeset= which removes the snapshot suffix

Oh…OK. That was never intended to be used in this way, but I guess it could work?

Could we achieve the same as MBR

You mean MRP?

without using MBR [MRP?]

Well, sure, it is just a tool.

and no junk commits

Only if you can mechanically generate the version number you want for a given commit during the build itself: #19 (comment)

it seems possible for JEP 229 use case to produce a POM that PCT can understand

Because it (actually JEP-305) is doing the tricky thing, as above.

Is it because PCT require a POM without properties or flatten values.

No, it can of course use a POM with properties and non-flattened value, but when you check out the tag or commit given by /project/scm/tag and run mvn -Dset.changelist -Pquick-build package or whatever, you should be producing the same content & version as what was in the release.

@jetersen
Copy link
Collaborator

jetersen commented Jul 13, 2022

Only if you can mechanically generate the version number you want for a given commit during the build itself:

Release drafter is capable of producing version number based on labels used for the release.
Perhaps that could be used? Since we are already using it to determine interesting categories.

And release drafter supports semver.

It will default the bump to whatever default you set in your release-drafter config:

https://github.com/release-drafter/release-drafter/blob/f4824e6ad8f4ac1e687db104bc43dfef0febaba7/.github/release-drafter.yml#L26

Also with config you can do major.minor and default to bumping minor

The tag can be read from github action output even.

@justusbunsi
Copy link

Oh…OK. That was never intended to be used in this way, but I guess it could work?

That's true. It was never intended in this repository. That's why I forked it and added the switch to opt-out this changeset. There was #13 to include it but I understand why that PR was closed.

@jglick
Copy link
Contributor Author

jglick commented Jul 13, 2022

Release drafter is capable of producing version number based on labels used for the release.
Perhaps that could be used?

Not for this purpose.

@jglick
Copy link
Contributor Author

jglick commented Jul 13, 2022

To reiterate, you can either

  • Generate a version number from inside a Maven extension that runs before the POM is resolved (maybe before it is even parsed) using only information that can be reconstructed from the source tree and Git history, but not Git references (much less some metadata on GitHub), as JEP-305 (and thus also JEP-229) does, and as Support automatic semVer with conventionalCommits #19 (comment) proposed might be theoretically possible for SemVer (given some rigor in how you handle commits); or
  • Push a pair of junk commits, like this PR lets MRP do.

@jetersen
Copy link
Collaborator

jetersen commented Jul 13, 2022

Not for this purpose.

I take it when you mean during the build itself your referring to the maven build and not the overall GitHub action build?
Hence why release-drafter cannot be used for this purpose.

I don't quite understand why not.

@jetersen
Copy link
Collaborator

What if I managed to finish release-drafter/release-drafter#1173 you would get a CLI, you could than wrap that CLI in maven extension and bobs your uncle? 😅

@jglick
Copy link
Contributor Author

jglick commented Jul 14, 2022

you're referring to the maven build

Yes.

@jglick
Copy link
Contributor Author

jglick commented Dec 15, 2022

I do not recommend it to begin with and only developed it because I am still hesitant to move away from MRP on parent POMs specifically

jenkinsci/plugin-pom#627 (comment)

@jglick jglick closed this Dec 15, 2022
@jglick jglick deleted the mrp branch December 15, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants