-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Keep tide
batch in pending
state when presubmit contexts of its PRs are pending
#32416
base: master
Are you sure you want to change the base?
Conversation
Hi @oliver-goetz. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oliver-goetz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
If we're going to add this it needs to be configurable and should not be the default behavior. I think the alternative proposal of overriding status contexts before a batch merges would be more appropriate if we are required or motivated to avoid having Tide override branch protection. That also seems like what was agreed upon in the issue: #32097 (comment) |
How does this work, if I'm fine with an configuration option. It could be the default behavior when
Isn't this how
Here it is a bit ugly that you have a context in |
I didn't think so, but maybe. I guess that is separate from what you're trying to change here in any case since we're specifically concerned with pending contexts. Maybe this could be improved separately, by triggering a new batch, but still potentially respecting the results from the old one if it succeeds.
I agree that would be confusing. Instead of linking to the failed prow job we should link to the successful batch result instead. |
PR needs rebase. 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. |
Ok, I'll see what I can do. |
While investigating the option overwrite option, I found another issue if we decide to cancel the pending prow jobs. The jobs which are going to be canceled are presubmit jobs, so
While we could wait in We could keep the pending prow jobs running, but still overwrite their contexts in |
I've just seen that some reporters (github, gerrit) do not report a prowjob when |
Fixes #32097
With this PR
tide
keeps batches inpending
state as long as required presubmit contexts of its member PRs are inpending
state too. Before, Github branch-protection rules could prevent merging PRs from a batch (see #32097 for details).The alternative approach listed in the issue (overwriting the
pending
contexts withsuccess
) was not implemented to keeptide
s behavior in regards of failing tests consistent.At the moment
tide
filters a PR with failed required contexts out of the subpool, so batches where this PR is member of cannot be merged. If we overwrite these contexts,tide
could merge PRs with failing contexts, when they fail late enough and their batch is already in the merge pool.