chore: add required review on master #12694
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,3 +52,24 @@ github: | |
| squash: true | ||
| merge: false | ||
| rebase: false | ||
|
|
||
| protected_branches: | ||
| master: | ||
| required_status_checks: | ||
| # strict means "Require branches to be up to date before merging". | ||
| strict: false | ||
| # contexts are the names of checks that must pass | ||
| contexts: | ||
| - Docker/build | ||
| - Frontend/build | ||
| - PR Lint/check | ||
| - E2E/Cypress | ||
| - lint | ||
| - test-mysql | ||
| - test-postgres | ||
| - test-sqlite | ||
|
|
||
| required_pull_request_reviews: | ||
| dismiss_stale_reviews: true | ||
|
There was a problem hiding this comment. Do we want this? It might cause some pain, say if a PR is approved but a rebase is needed before merging. |
||
| require_code_owner_reviews: true | ||
| required_approving_review_count: 1 | ||
|
There was a problem hiding this comment. definitely would like a second pair of eyes on all this. I don't think there's a way to test before merging. There was a problem hiding this comment. The only risk I see is the contexts are not correct. Let's just be ready to fix/revert if needed There was a problem hiding this comment. Thanks, yeah, I set up: #12698 and will rebase when this lands. We might as well wait until next week, too, to land this. |
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little unclear which names should the contexts use.
This API returns only the job name, with Matrix suffix:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ktmud do you have these actions set up in your fork? If so, can you check the name that appears under the settings tab for the repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming the API will use the same names as displayed here?
We might want to rename a couple of our jobs to avoid confusion (there are a couple of duplicate "build"s).
I guess the safest route would be to have a very limited list here to override the admin settings, then add them back in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those are the names we need to use, though they do look quite different than the names reported on PRs. I don't think the risk here is that high, I doubt committers will be merging PRs with failing status checks (even if they're not actually marked required). We could make and announcement just in case, "You might see some changes to the required checks as we figure things out, please don't merge anything that's failing a check"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs show them as
<Action Name>/<Job Name>:although I realize that the action name was missing in some of the tests when I pulled them in, so I can update that. Looks to me that we would have to provide the action name in order for the duplicated job names to even work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the risk here is that if you add checks not matching the actual context names as required, everyone will (may) be blocked from merging until INFRA intervenes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's a good point. We should probably do this during hours of low activity. Worst case, we revert quickly and none of the check will be required. Without the admin repo privileges we are kind of shooting in the dark though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following on this scenario, do you think all the "blocked" prs would automatically be mergable after we revert this? Do you think there's any risk that we won't be able to unblock them? I think worst case scenario, we should be able to close and open the pr to retrigger the checks, which would just be time consuming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think rather than revert we'd want to set the context as empty or remove the ones that end up stuck