Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Oct 5, 2018

Currently things like openshift/installer#415 and openshift/installer#425 that try to unstick us vs. some external change we need to /hold the other approved PRs to get them out of the merge queue while the fix goes in. With the bot removed from our repository, those PRs would remove themselves as they failed naturally, and we'd just /retest them after the fix lands. We can turn the bot back on once we got back to one-external-workaround a week or so, vs. our current several per day ;).

Docs for the repo: syntax are here.

/assign @smarterclayton

Currently things like [1,2] that try to unstick us vs. some external
change we need to /hold the other approved PRs to get them out of the
merge queue while the fix goes in.  With the bot removed from our
repository, those PRs would remove themselves as they failed
naturally, and we'd just /retest them after the fix lands.  We can
turn the bot back on once we got back to one-external-workaround a
week or so, vs. our current several per day ;).

Docs for the repo: syntax are in [3].

[1]: openshift/installer#415
[2]: openshift/installer#425
[3]: https://help.github.com/articles/searching-issues-and-pull-requests/#search-within-a-users-or-organizations-repositories
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wking
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 5, 2018
@stevekuznetsov
Copy link
Contributor

NACK

/hold

If you want something to jump the queue, just push the green button. With batching, I can't imagine your queue would ever be longer than a batch or two. This really feels like a bandaid for a symptom of a larger problem that's better solved at the root.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2018
@wking
Copy link
Member Author

wking commented Oct 7, 2018

If you want something to jump the queue, just push the green button.

This is not an option. Even when everything but Tide is green, I see:

Merging is blocked
The base branch restricts merging to authorized users. Learn more about protected branches.

Elevating my permissions to avoid that would be even more of a cludge than this.

With batching, I can't imagine your queue would ever be longer than a batch or two.

Say you have PRs A, B, and C in flight, with C fixing some issue. I don't want Tide waiting on the likely-to-fail A and B retests while it collects a batch. I just want C landed as quickly as possible. But I'm not clear on how tide collects batches, are there docs on that somewhere?

Another issue is that the robot doesn't look into the failure before retesting. This means it takes longer to surface breakage like "some operator pushed a broken image" while the bot blindly retests a PR that's just tweaking installer docs (like this).

@stevekuznetsov
Copy link
Contributor

Elevating my permissions to avoid that would be even more of a cludge than this.

Your team lead and group lead should both have write access to the repo. That is the intended way to merge changes quickly.

how tide collects batches

tide will find all of the PRs that are valid to consider for merge, as described by the configuration -- for instance, this usually means passing tests, having lgtm and approved labels, and not having the various do-not-merge/ labels. Then, tide always tests the oldest PR from that set; if multiple PRs in the set exist that can be merged together, a batch is formed. We ensure that the singleton test and the batch don't race -- if the singleton finishes successfully, but the batch is still running, we wait for the batch and hope to merge more PRs; if the batch fails, we still merge the singleton. You'll never regress to anything worse than a serial queue with batching.

@stevekuznetsov
Copy link
Contributor

Said another way, tide is never waiting for retests, so the content of this PR will not change the merge behavior.

@stevekuznetsov
Copy link
Contributor

@smarterclayton we were supposed to have ratcheting dependencies. Please let's not recreate what openshift-ansible had to deal with in the first installer tests two years ago. Why are breaking changes being pushed out to the latest release stream? How can we stop that?

@wking wking closed this Oct 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants