Introduce an option to overwrite GitHub contexts of pending presubmit ProwJobs when merging batches in `tide` #135

oliver-goetz · 2024-04-25T15:47:42Z

Fixes #209

With this PR tide overwrites the context of pending ProwJobs when it merges a batch of PRs. It uses the status of the succeeded batch job with the same context. The feature is disabled by default and can be enabled via overwrite-pending-contexts context option.
When the context is overwritten, tide deletes any test report comments created by crier (ref) of this PR which might still exist. They contain previous failed runs of the contexts which have just been overwritten. It would be confusing to keep them in the PR.
Before, Github branch-protection rules could prevent merging PRs from a batch (see kubernetes/test-infra#32097 for details).

When a PR is merged the ProwJobs which are still running are canceled automatically by trigger plugin of hook, so there is no need to cancel them in tide.

This PR replaces kubernetes/test-infra#32416.

k8s-ci-robot · 2024-04-25T15:47:51Z

Welcome @oliver-goetz!

It looks like this is your first PR to kubernetes-sigs/prow 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/prow has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-04-25T15:47:51Z

Hi @oliver-goetz. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

netlify · 2024-04-25T15:47:56Z

✅ Deploy Preview for k8s-prow ready!

Name	Link
🔨 Latest commit	`bf0e9aa`
🔍 Latest deploy log	https://app.netlify.com/sites/k8s-prow/deploys/66f59c0d86af250008b84d5c
😎 Deploy Preview	https://deploy-preview-135--k8s-prow.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

stevekuznetsov · 2024-04-29T21:04:06Z

pkg/tide/github.go

+
+	for _, pj := range pjs {
+		for _, pull := range pj.Spec.Refs.Pulls {
+			if pr, found := prNums[pull.Number]; found && pr.HeadRefOID == pull.SHA {


Do we need to also consider the ref for the target branch? Could we have some race like:

trigger presubmits on target ref A

trigger batch for same

push to PR

trigger presubmits for target ref B

presubmits publish pending statuses

batch finishes

this code runs, overwriting presubmits

batch would have been cancelled by the new push

Okay, I see.
I restructured this function that it checks if all PRs of a batch job are valid before collecting it. The logic is similar to how batch PRs are accumulated (ref).
Now, the context of a batch job which contain invalid or missing PRs are not collected anymore.

jihoon-seo · 2024-05-07T08:26:11Z

/ok-to-test

oliver-goetz · 2024-05-17T07:08:24Z

@stevekuznetsov could you check this PR again please? 🙂

oliver-goetz · 2024-06-05T15:34:01Z

Now, any leftover test report comments will be deleted too, when the context was overwritten.
This avoids situations like shown in the screenshot where the context is marked as SUCCESS but there is still a comment with a link to a failed test.

It might happen when the batch jobs succeed and there is a failing test in the trigger PR, which is restarted fast enough while the batch job is still running.

oliver-goetz · 2024-06-17T10:47:48Z

friendly reminder, please review this PR 😄

petr-muller · 2024-07-18T09:14:10Z

/cc

oliver-goetz · 2024-09-02T07:42:44Z

ping 🔔
Could someone review this PR please.

krzyzacy · 2024-09-12T21:15:34Z

pkg/tide/github.go

@@ -265,6 +270,17 @@ func (gi *GitHubProvider) mergePRs(sp subpool, prs []CodeReviewCommon, dontUpdat
 			continue
 		}

+		// Overwrite context of pending presubmit prow jobs if enabled


Do we ever plan to overwrite the context to something other than success? I would consider either make that explicit in the function name, or make it take in the override status?

I don't think it makes sense to set the status to anything else than success in the tide context. I updated the name of the function.

krzyzacy · 2024-09-26T16:30:02Z

pkg/tide/github.go

+				prowJobsForPRs[pull.Number].successfulBatchJob[pj.Spec.Context] = pj
+			}
+		case prowapi.PresubmitJob:
+			if jobState != pendingState {


do we also want to override failed presubmit runs if the batch job finishes with success?

If a presubmit run of a batch member fails, the batch jobs continue to run. However, the batch is removed from the merge pool, so it would never reach this point here.

krzyzacy · 2024-09-26T16:33:04Z

@stevekuznetsov @petr-muller wanna skim through again as you are the original reviewers?

petr-muller · 2024-09-30T12:12:41Z

Let's not block on me, I'll skim post-merge (eventually :D ) Thanks @krzyzacy for reviewing this one, it's great to see you back. And thanks @oliver-goetz for your submission and patience 🙇

/lgtm

k8s-ci-robot · 2024-09-30T12:45:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oliver-goetz, petr-muller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [petr-muller]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from cjwagner and stevekuznetsov April 25, 2024 15:47

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 25, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 25, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 25, 2024

stevekuznetsov reviewed Apr 29, 2024

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 2, 2024

oliver-goetz requested a review from stevekuznetsov May 2, 2024 15:39

oliver-goetz force-pushed the enh/tide-batch-merge branch from 7a15b1f to 8d7d484 Compare May 15, 2024 06:02

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024

BenTheElder mentioned this pull request Jul 17, 2024

Keep tide batch in pending state when presubmit contexts of its PRs are pending kubernetes/test-infra#32416

Closed

k8s-ci-robot requested a review from petr-muller July 18, 2024 09:14

oliver-goetz mentioned this pull request Jul 18, 2024

Branch protection rules prevent first PR of a merge-batch from being merged #209

Closed

krzyzacy reviewed Sep 12, 2024

View reviewed changes

oliver-goetz requested a review from krzyzacy September 26, 2024 16:07

krzyzacy reviewed Sep 26, 2024

View reviewed changes

oliver-goetz added 6 commits September 26, 2024 19:33

Ignore folders of common IDEs

1554f59

Introduce OverwritePendingContexts config option for tide

6c1c22f

Add OverwritePendingContexts functionality to tide

4d83b73

Consider batch jobs where all PRs are valid only

62a2f01

Delete test report issue comments when overwriting the contexts of a PR

5a70fa6

Address PR review feedback

bf0e9aa

oliver-goetz force-pushed the enh/tide-batch-merge branch from 26c1f27 to bf0e9aa Compare September 26, 2024 17:38

k8s-ci-robot assigned petr-muller Sep 30, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2024

petr-muller approved these changes Sep 30, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 30, 2024

k8s-ci-robot merged commit 8302b5c into kubernetes-sigs:main Sep 30, 2024
11 checks passed

oliver-goetz deleted the enh/tide-batch-merge branch October 1, 2024 09:21

oliver-goetz mentioned this pull request Oct 1, 2024

Activate overwrite-pending-contexts feature for tide gardener/ci-infra#2466

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce an option to overwrite GitHub contexts of pending presubmit ProwJobs when merging batches in `tide` #135

Introduce an option to overwrite GitHub contexts of pending presubmit ProwJobs when merging batches in `tide` #135

oliver-goetz commented Apr 25, 2024 •

edited

Loading

k8s-ci-robot commented Apr 25, 2024

k8s-ci-robot commented Apr 25, 2024

netlify bot commented Apr 25, 2024 •

edited

Loading

stevekuznetsov Apr 29, 2024

oliver-goetz May 2, 2024

jihoon-seo commented May 7, 2024

oliver-goetz commented May 17, 2024

oliver-goetz commented Jun 5, 2024

oliver-goetz commented Jun 17, 2024

petr-muller commented Jul 18, 2024

oliver-goetz commented Sep 2, 2024

krzyzacy Sep 12, 2024

oliver-goetz Sep 26, 2024

krzyzacy Sep 26, 2024

oliver-goetz Sep 26, 2024

krzyzacy commented Sep 26, 2024

petr-muller commented Sep 30, 2024

k8s-ci-robot commented Sep 30, 2024

Introduce an option to overwrite GitHub contexts of pending presubmit ProwJobs when merging batches in tide #135

Introduce an option to overwrite GitHub contexts of pending presubmit ProwJobs when merging batches in tide #135

Conversation

oliver-goetz commented Apr 25, 2024 • edited Loading

k8s-ci-robot commented Apr 25, 2024

k8s-ci-robot commented Apr 25, 2024

netlify bot commented Apr 25, 2024 • edited Loading

✅ Deploy Preview for k8s-prow ready!

stevekuznetsov Apr 29, 2024

Choose a reason for hiding this comment

oliver-goetz May 2, 2024

Choose a reason for hiding this comment

jihoon-seo commented May 7, 2024

oliver-goetz commented May 17, 2024

oliver-goetz commented Jun 5, 2024

oliver-goetz commented Jun 17, 2024

petr-muller commented Jul 18, 2024

oliver-goetz commented Sep 2, 2024

krzyzacy Sep 12, 2024

Choose a reason for hiding this comment

oliver-goetz Sep 26, 2024

Choose a reason for hiding this comment

krzyzacy Sep 26, 2024

Choose a reason for hiding this comment

oliver-goetz Sep 26, 2024

Choose a reason for hiding this comment

krzyzacy commented Sep 26, 2024

petr-muller commented Sep 30, 2024

k8s-ci-robot commented Sep 30, 2024

Introduce an option to overwrite GitHub contexts of pending presubmit ProwJobs when merging batches in `tide` #135

Introduce an option to overwrite GitHub contexts of pending presubmit ProwJobs when merging batches in `tide` #135

oliver-goetz commented Apr 25, 2024 •

edited

Loading

netlify bot commented Apr 25, 2024 •

edited

Loading