Skip PCT by default on PRs #2034

jglick · 2023-05-02T15:44:53Z

Simpler alternative to #2031 to consider. By comparison, it retains the straightforward release-from-master behavior, so CD configuration should not need any adjustment, and there is no need to track the status of release branch merges. Also keeps #1993, so PCT will be run only when you ask for it in a PR, or explicitly test master (perhaps when planning a release). We can also consider running master builds on a schedule, such as @nightly.

I believe it is unnecessary to use a dedicated release branch. Such a system makes sense for repositories which:

Have a lot of untested code coming in which is likely to cause frequent test failures or genuine regressions.
Must be able to be released on short notice, for example to address a security vulnerability.
Cannot tolerate much risk of regression.

None of those criteria seem to apply to a developer tool like a BOM which after all is merely a convenience to help you manage a dependency list. If some change introduced a PCT regression, we can generally take our time fixing it, whether by releasing some plugin with a test or behavioral change and integrating it; reverting the problematic update; or adding a test to an exclusion list.

jglick · 2023-05-02T15:46:06Z

Jenkinsfile

-          launchable("record tests --session ${session} --group ${repository} maven './**/target/surefire-reports' './**/target/failsafe-reports'")
+if (BRANCH_NAME == 'master' || env.CHANGE_ID && pullRequest.labels.contains('full-test')) {
+  branches = [failFast: false]
+  lines.each {line ->


Hide whitespace to see real change

jglick · 2023-05-02T15:47:06Z

Jenkinsfile

-          def session = readFile(sessionFile).trim()
-          launchable("record tests --session ${session} --group ${repository} maven './**/target/surefire-reports' './**/target/failsafe-reports'")
+if (BRANCH_NAME == 'master' || env.CHANGE_ID && pullRequest.labels.contains('full-test')) {
+  branches = [failFast: false]


equivalently,

Suggested change

branches = [failFast: false]

branches = [:]

jglick · 2023-05-02T15:47:43Z

updatecli/updatecli.d/plugin-compat-tester.yml

@@ -43,3 +43,4 @@ actions:
    spec:
      labels:
        - dependencies
+        - full-test


If we are updating PCT, presumably we want to run it!

jglick · 2023-05-02T15:51:26Z

Could also consider adding full-test to

bom/.github/dependabot.yml

Lines 3 to 9 in 29b6f89

    
           - package-ecosystem: "maven" 
        
             open-pull-requests-limit: 10 
        
             directory: "/sample-plugin" 
        
             reviewers: 
        
               - "jglick" 
        
             schedule: 
        
               interval: "daily"

so that core bumps run PCT. Would want to suppress this on plugin-pom bumps as well as groovy-maven-plugin and groovy-all; not sure if DB makes that possible.

jglick · 2023-05-02T15:55:15Z

Could also consider .github/workflows/dependabot-automerge.yml as seen in #2031, though we would need to then be somewhat more careful about what is going into a release. I tend to glance at plugin release notes before approving PRs just out of interest, but it would be really unusual to specifically reject such a bump if it passes CI.

jglick · 2023-05-02T15:59:23Z

We could also revert #2032 for the prep stage, keeping it only for the pct branches, so that PRs get tested more promptly.

basil

This places the burden of running the full test suite and dealing with the failures on whoever happens to be cutting a release, whenever they happen to be cutting a release. I think this negatively incentivizes running tests and cutting releases. We want to create positive incentives for maintainership duties, not negative ones.

In contrast, my proposal creates positive incentives for maintainership duties by having automation open PRs to run automated tests early (before release) and often (three times a week). More positive incentives are created by opening PRs (thus allowing for group discussion), which sends out notifications and thus urges people to take action early (as opposed to this proposal, in which nobody would notice if a test started failing until they happened to be interested in doing a release).

The only situation in which I would accept your proposal is one in which you volunteer to regularly (at least once a month) run the build, deal with failures, re-run the build if necessary, and cut the release. And this would mean doing the work, not leaving comments about what "needs to be" done.

dduportal · 2023-05-02T16:49:11Z

We could also revert #2032 for the prep stage, keeping it only for the pct branches, so that PRs get tested more promptly.

🤔 did you mean #2031 (instead of #2032)? I'm not sure to see the link with using another label (to run on another node pool) with your proposal here?

jglick · 2023-05-02T16:59:10Z

@dduportal no, I meant #2032: use the maven-bom label for pct-* branches, but revert to maven-11 for the quick prep stage. Somewhat orthogonal and could be done independently; I just noticed here that the build was being held up waiting the dedicated node pool, which is only really needed for the much more expensive PCT runs.

MarkEWaite · 2023-05-02T18:00:30Z

I have some preference for #2031 but would also be willing to use the workflow proposed in this pull request. I agree with the observation from @basil that this method places a greater burden on the maintainer that generates a release, but I think that can be acceptable if we have some form of rotation of maintainers that are working to assure a release happens at least once a week.

I suggest a release at least once a week because in the past 40 BOM releases we've averaged 13 commits per release and we easily have 13 pull requests per week.

basil · 2023-05-02T18:07:39Z

I agree with the observation from @basil that this method places a greater burden on the maintainer that generates a release

Not just the maintainer that performs a BOM release, but also anyone who happens to need to do a full test run: e.g. someone doing unrelated core/PCT work that happens to require a full test run. Unless a full test run is regularly scheduled and unless BOM maintainers are notified to take action when it fails, it is possible that we would fall behind on these fronts and that the unlucky person doing a BOM release or a core/PCT change would have to face the consequences (which would negatively incentivize them to do such work).

basil · 2023-05-02T18:14:31Z

I think that can be acceptable if we have some form of rotation of maintainers that are working to assure a release happens at least once a week.

A rotation would alleviate my concern about negative incentives. If the other maintainers agree to form such a rotation, I would support the approach in this PR and would even be willing to participate as one of the members of the rotation. If the other maintainers want to proceed with this PR but will not commit to such a rotation, then I would remain hesitant about the approach in this PR.

I suggest a release at least once a week

Yes, a once a week cadence (similar to weekly core releases) sounds about right to me for the reasons you gave.

timja · 2023-05-02T18:49:13Z

I do not support a rotation / commitment on this.

jglick · 2023-05-02T22:31:57Z

I was not proposing any sort of rotation, whatever that means.

I sketched a way for trunk to be automatically tested on a regular schedule (weekly, as an example) that ought to deliver the usual GitHub notifications, as well as providing a placeholder PR that could be self-assigned, used to collect notes about bisections in progress, etc. It could be amended with actual fixes I suppose, or simply be closed and real PRs opened (with full-test) from forks. Like most other things involving GHA workflows, it is not straightforward to test this in advance (the workflow_dispatch is for debugging).

I had hoped to be able to open a PR with no commits, which would be beneficial since if it passed then master would already be green and so you could proceed straight to release, but it seems GH goes out of its way to prevent this. (If you try to trick it by resetting the head branch to the base branch, it marks the PR closed.) Would be possible, but a bit more effort, to rerun the master head and then have another workflow triggered on a failing check from it which would (re-)open an issue.

Just an option. Probably not worth spending too much time debating design since it is simple enough to revert or modify any proposal if it is not working out the way it was hoped.

basil

I was not proposing any sort of rotation, whatever that means.

Yeah I know, my point was that some positive incentive is needed for the system to sustain itself, and it was missing in the first version of this PR. That would have led to a negative incentive for doing certain types of work, hence my negative feedback about the first version of this PR. In my mind the problem can be solved by creating a positive incentive to test early and often, either by creating automation that runs builds and pings people regularly or by having the maintainers commit to doing this manually. Since this PR now creates a positive incentive in the form of cron-scheduled builds and an automated PR, my concern is now alleviated. And since it is simpler than #2031, I think I prefer this PR to #2031.

jetersen · 2023-05-03T08:48:04Z

I like simplicity over what #2031 was suggesting 😅

jetersen

One minor issue otherwise LGTM.

.github/workflows/run-full-test.yaml

MarkEWaite

Looks good to me.

The suggested change from @jetersen seems like a good safety measure by always deleting the full-tests branch whether or not it has been pushed. I was unable to create a condition where the git branch -d full-tests would fail, but there may be such a case. Pull request is approved whether or not the suggestion is accepted.

basil · 2023-05-03T14:44:57Z

Still a bit incomplete without the following portions that were discussed:

Dependabot auto-merge of dependencies
Use regular container agents rather than BOM agents for prep.sh stage.

+0 from me in this incomplete state. This would be a +1 if these action items were completed.

Co-authored-by: Joseph Petersen <me@jetersen.dev>

jglick · 2023-05-03T15:00:25Z

Dependabot auto-merge of dependencies

#2034 (comment) but sure, fine with me to add that here.

Use regular container agents rather than BOM agents for prep.sh stage

Yes, could be done independently but may as well prepare it here since it seems there is consensus we should try this approach.

Will try to do both today.

dduportal · 2023-05-03T15:12:02Z

@dduportal no, I meant #2032: use the maven-bom label for pct-* branches, but revert to maven-11 for the quick prep stage. Somewhat orthogonal and could be done independently; I just noticed here that the build was being held up waiting the dedicated node pool, which is only really needed for the much more expensive PCT runs.

Use regular container agents rather than BOM agents for prep.sh stage.

+0 on this: I'm not in favor neither I'm against this:

Keeping all bom builds in the "BOM" node pools is easier to mentally map when dealing with costs and setup on infrastructure side.
Moving the "prep" phase on the "NOT bom" node pool increase partially the probability to immediately have an agent instead of having to wait for a node scale up (but only partially: if you have 3, 6, 9 plugins being built, then you'll wait the same time as it would trigger a scale up.

I don't see a reason to block this PR for this chore.

Thanks for the work, proposals, reviews and solutions on this and on #2031 .

basil · 2023-05-03T15:14:32Z

The prep.sh phase is pretty light and is effectively equivalent to a normal plugin build. It's only the PCT phase that is heavyweight, massively parallel, and requires a dedicated node pool. The "BOM" node pool could conceptually be thought of as the "PCT" node pool from the perspective of resource requirements.

…enkinsci#2034 (comment)

jglick · 2023-05-03T19:59:37Z

I think this is ready to go if there are no objections from the last couple of commits? We can see how it goes for a couple of weeks and adjust as needed.

MarkEWaite · 2023-05-03T20:40:07Z

I've started a separate build of the master branch in hopes that the DNS resolution failures won't hit this time. Infra team is investigating those failures in jenkins-infra/helpdesk#3559

jglick · 2023-05-04T11:53:45Z

.github/workflows/dependabot-automerge.yaml

+    runs-on: ubuntu-latest
+    if: ${{ github.actor == 'dependabot[bot]' }}
+    steps:
+      - name: Enable auto-merge for Dependabot PRs


Working on #2036, #2038, #2039.

.github/workflows/run-full-test.yaml

jglick · 2023-05-08T19:38:59Z

.github/workflows/run-full-test.yaml

+        git commit --allow-empty --message 'Phony commit'
+        git push origin full-tests
+        # Not using --draft to ensure notifications are sent:
+        gh pr create --head --title 'Testing master (do not merge)' --body 'Close this PR if it passes; otherwise please fix failures.' --reviewer jenkinsci/bom-developers --label full-test


Tested in #2052.

Skip PCT by default on PRs

57817f6

jglick added the chore Reduces future maintenance label May 2, 2023

jglick requested a review from a team May 2, 2023 15:44

jglick commented May 2, 2023

View reviewed changes

basil requested changes May 2, 2023

View reviewed changes

jglick mentioned this pull request May 2, 2023

Implement a staging workflow to reduce costs #2031

Closed

jglick added 2 commits May 2, 2023 18:15

Run full tests weekly

52817f7

Empty commit should suffice

809cd9a

basil approved these changes May 3, 2023

View reviewed changes

timja approved these changes May 3, 2023

View reviewed changes

jetersen approved these changes May 3, 2023

View reviewed changes

.github/workflows/run-full-test.yaml Outdated Show resolved Hide resolved

MarkEWaite approved these changes May 3, 2023

View reviewed changes

git branch -D might be necessary

5fee7da

Co-authored-by: Joseph Petersen <me@jetersen.dev>

jglick added 2 commits May 3, 2023 11:48

Copying DB automerge workflow from jenkinsci#2031

f639628

Revert jenkinsci#2032 for prep phase, retaining node pool for PCT j…

ca28da0

…enkinsci#2034 (comment)

jglick added the full-test Test all LTS lines in this PR and do not halt upon first error. label May 3, 2023

MarkEWaite merged commit 06df805 into jenkinsci:master May 3, 2023
473 checks passed

jglick deleted the miser branch May 3, 2023 21:02

jglick commented May 4, 2023

View reviewed changes

jglick mentioned this pull request May 4, 2023

Squash Dependabot PRs #2040

Merged

jglick commented May 7, 2023

View reviewed changes

.github/workflows/run-full-test.yaml Show resolved Hide resolved

This was referenced May 8, 2023

Set Git user #2047

Merged

Missing argument to --head #2049

Merged

Drop --reviewer #2050

Merged

jglick commented May 8, 2023

View reviewed changes

jglick mentioned this pull request May 13, 2023

Automatically mark trunk passing if full test does #2069

Open

jglick mentioned this pull request Jul 12, 2023

Bump workflow-api-plugin.version from 1219.v05cd837ea_249 to 1232.v1679fa_2f0f76 in /bom-weekly #2260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip PCT by default on PRs #2034

Skip PCT by default on PRs #2034

jglick commented May 2, 2023

jglick May 2, 2023

jglick May 2, 2023

jglick May 2, 2023

jglick commented May 2, 2023

jglick commented May 2, 2023

jglick commented May 2, 2023

basil left a comment •

edited

dduportal commented May 2, 2023

jglick commented May 2, 2023

MarkEWaite commented May 2, 2023

basil commented May 2, 2023

basil commented May 2, 2023

timja commented May 2, 2023

jglick commented May 2, 2023

basil left a comment

jetersen commented May 3, 2023

jetersen left a comment

MarkEWaite left a comment

basil commented May 3, 2023

jglick commented May 3, 2023

dduportal commented May 3, 2023

basil commented May 3, 2023

jglick commented May 3, 2023

MarkEWaite commented May 3, 2023

jglick May 4, 2023

jglick May 8, 2023

Skip PCT by default on PRs #2034

Skip PCT by default on PRs #2034

Conversation

jglick commented May 2, 2023

jglick May 2, 2023

Choose a reason for hiding this comment

jglick May 2, 2023

Choose a reason for hiding this comment

jglick May 2, 2023

Choose a reason for hiding this comment

jglick commented May 2, 2023

jglick commented May 2, 2023

jglick commented May 2, 2023

basil left a comment • edited

Choose a reason for hiding this comment

dduportal commented May 2, 2023

jglick commented May 2, 2023

MarkEWaite commented May 2, 2023

basil commented May 2, 2023

basil commented May 2, 2023

timja commented May 2, 2023

jglick commented May 2, 2023

basil left a comment

Choose a reason for hiding this comment

jetersen commented May 3, 2023

jetersen left a comment

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

basil commented May 3, 2023

jglick commented May 3, 2023

dduportal commented May 3, 2023

basil commented May 3, 2023

jglick commented May 3, 2023

MarkEWaite commented May 3, 2023

jglick May 4, 2023

Choose a reason for hiding this comment

jglick May 8, 2023

Choose a reason for hiding this comment

basil left a comment •

edited