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 | ||
| - Python Misc/lint | ||
| - Python MySQL/test-mysql | ||
| - Python MySQL/test-postgres | ||
| - Python MySQL/test-sqlite | ||
|
|
||
| required_pull_request_reviews: | ||
| dismiss_stale_reviews: false | ||
| 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.
@nytai @ktmud Do we feel comfortable with this list or do we want to use the api response instead?
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 found a few other projects using this feature
https://github.com/apache/skywalking-eyes/blob/main/.asf.yaml
https://github.com/apache/skywalking/blob/master/.asf.yaml
https://github.com/apache/skywalking-python/blob/master/.asf.yaml
It seems they're using the portion after the
/, which would result in duplicates for us. I think this looks good to meThere 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 suspect if a match isn't found in the list of recent contexts nothing happens
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 suspect it will wait for that check forever... Maybe let's add a very short list to override the admin settings first, then gradually add them back?
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.
When you go to settings you can't just enter any string, you have to choose from the list of "status checks found in the last week for this repository". I think it needs to be a context the repo has seen before. Also, here they have
buildset but no check is marked as required.https://github.com/apache/skywalking-client-js/blob/04dd6b3b024f838b3b92002a370ce3b7f70638b4/.asf.yaml#L38
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.
Although, it may be a good idea to be extra safe here as we may end up in a stuck state waiting for apache infra.
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.
Tested with a personal fork, now I'm 90% sure GitHub will just be forever waiting for these unidentifiable checks:
IMO the safest course of actions is:
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.
ah, this is great. @ktmud it looks like three of four of those context strings didn't match any existing checks. Did "build" work and match to
Frontend / build?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.
Actually based on your other screenshot, I think I should go with the whole string with the space
PR Lint / checkfor example.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.
Thanks for the thorough investigation @ktmud. I think you're right, this file is just used to make a request to the github api.