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

Suppressing builds on master branch #1993

Merged
merged 8 commits into from Apr 21, 2023

Conversation

jglick
Copy link
Member

@jglick jglick commented Apr 21, 2023

Trying to minimize load on ci.jenkins.io by only building PRs by default; if and when you want to run a release, use Re-run in GitHub to do a master build that could trigger CD. jenkins-infra/helpdesk#3521 (comment)

@jglick jglick added the chore Reduces future maintenance label Apr 21, 2023
@jglick
Copy link
Member Author

jglick commented Apr 21, 2023

https://github.com/jenkinsci/bom/pull/1993/checks?check_run_id=12935716671 jenkins.branch.BranchEventCause vs. https://github.com/jenkinsci/bom/pull/1993/checks?check_run_id=12935728839 io.jenkins.plugins.checks.github.CheckRunGHEventSubscriber$GitHubChecksRerunActionCause as expected

@jglick jglick requested review from dduportal and a team April 21, 2023 21:03
@jglick jglick marked this pull request as ready for review April 21, 2023 21:07
Copy link
Member

@lemeurherve lemeurherve left a comment

Choose a reason for hiding this comment

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

When planning for your help desk suggestion I was thinking about building on tag only if not on a pull request, I would not have though of this way, great trick!

Jenkinsfile Show resolved Hide resolved
README.md Show resolved Hide resolved
@jglick
Copy link
Member Author

jglick commented Apr 21, 2023

building on tag only if not on a pull request

Would not work, since a release tag is created only after a successful build.

Not bothering to wait for full build, since the rest of the build should be unaffected (except for fullTest, and I can manually verify here that just weekly and bom-2.361.x are being built).

@jglick jglick merged commit 48eb92f into jenkinsci:master Apr 21, 2023
5 of 6 checks passed
@jglick jglick deleted the termination-for-cause branch April 21, 2023 21:58
@jglick
Copy link
Member Author

jglick commented Apr 21, 2023

https://github.com/jenkinsci/bom/runs/12936800342 🎉

@lemeurherve
Copy link
Member

lemeurherve commented Apr 21, 2023

building on tag only if not on a pull request

Would not work, since a release tag is created only after a successful build.

I was thinking of a tag pushed manually. Probably a flawed reasoning, and less practical than your solution anyway.

@jglick
Copy link
Member Author

jglick commented Apr 21, 2023

a tag pushed manually

Oh I see. Could probably be made to work, though it would require some changes to multibranch config and is a bit further afield from how JEP-229 normally works.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Consensus regarding this change in maintenance practice does not appear to have been formed with other maintainers. If I were still maintaining this component, I would not accept such a lack of consensus.

@@ -1,5 +1,9 @@
properties([disableConcurrentBuilds(abortPrevious: true), buildDiscarder(logRotator(numToKeepStr: '7'))])

if (BRANCH_NAME == 'master' && currentBuild.buildCauses*._class == ['jenkins.branch.BranchEventCause']) {
error 'No longer running builds on response to master branch pushes. If you wish to cut a release, use “Re-run checks” from this failing check in https://github.com/jenkinsci/bom/commits/master'
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to error? can't we just skip the build with a return?

the build already passed on the PR before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do not fail the build then there is no failing check, and GH does not offer to rerun a passing check.

Should basically be cosmetic, but there may be some fancier way to create a dedicated failing check without actually failing the build. (Could then set the status to NOT_BUILT for example.) I am not sure if github-checks allows that using generic steps. Might work to use the publishChecks step.

Copy link
Member

Choose a reason for hiding this comment

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

do we need a failing check? you can create a pull request to test a build and it won't be on master if it hasn't passed on a pull request.

it's not ideal to open the repository and just see red.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

you can create a pull request to test a build and it won't be on master if it hasn't passed on a pull request.

Sorry, I did not follow that sentence.

it's not ideal to open the repository and just see red.

Of course, but I do not see an alternative. Even if the Jenkins build is marked successful, the alternate failing check trick I proposed in #1993 (comment) would still result in the commit being shown as failed.

To recap, the idea of this PR is to make master builds do nothing if triggered by SCM; but offer a mechanism by which someone with write permission to this repository yet no special permissions on ci.jenkins.io can somehow trigger a real build of master on demand, which if successful will then proceed to trigger a CD release as usual (depending on labels). The only way I could think of to make that happen is to make the last master commit be marked with some failing check, so that Re-run is offered, at which point we can inspect the build causes and decide to proceed. Letting the master build be marked successful means that only people with elevated permissions on the CI server (of whom I at least am not one) would be able to trigger bom releases, contrary to the basic goal of JEP-229. If you have a better idea, please do tell.

Note one effect of this change is that the next pseudorelease will not be populated by Release Drafter until the release is actually cut, since the stock CD workflow checks statuses first and only then runs R.D. If desired, we could clone & modify this workflow to run R.D. earlier, for example after every master build completes, whether successful or not.

Copy link
Member

Choose a reason for hiding this comment

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

what I mean though is why do we need to run a successful build of bom on master? i.e. with branch protection configured here it assures that it passed on the PR build and there's nothing different in this repo between PR and master can't we just auto release on master if labels match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see. That would be possible. There are two considerations though:

  • This PR ensures that the remaining master builds test all LTS branches, so it is actually safer than the prior setup. To replicate that safety with your proposed setup, the onus would be on whoever merged a PR with an “interesting” label to remember to run a build with the full-test label first.
  • Since Jenkins PR-merge strategy is long since disabled, and we are not using a GH merge queue, a passing PR-head build does not guarantee that all is well after the merge. It would then become possible for a bom release to be cut from a revision which is actually broken. We would notice pretty soon (when new DB PRs start consistently failing with the same error with no apparent relation to the diff) and issue a revert or correction, but it still makes me nervous. (For the same reason, the JEP-229 workflow generally checks master status, even though we expect it “should” be passing.)

I have no strong opinion, so if you want to experiment with another policy, go ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Reduces future maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants