Skip to content
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

bugfix for project's .asf.yaml #3451

Merged
merged 1 commit into from
Aug 20, 2022
Merged

Conversation

StevenLuMT
Copy link
Member

@StevenLuMT StevenLuMT commented Aug 19, 2022

Descriptions of the changes in this PR:

Motivation

using .asf.yaml to configure the project,
but there 's some config bugfix for project's .asf.yaml
the name of workflow's jobs is not set, so #3439 causes contexts can't match check's jobs

this picture is the other new add pr's running result: the other pr's workflow run abnormally
image

this picture is current running result: current pr is running normally because the job name is added in yaml file
image

Changes

  1. add the name for checks
  2. change required_approving_review_count to 2 person
  3. update README.md for workflows

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@@ -63,7 +63,7 @@ github:
required_pull_request_reviews:
dismiss_stale_reviews: false
require_code_owner_reviews: true
required_approving_review_count: 1
required_approving_review_count: 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong.
Otherwise committers won't be able to commit their own patches with only one approval from another committer.
The rule is that you need two +1 from people with committer karma.

So if a committer sends a patch,he needs only 1 binding +1

@StevenLuMT @zymap

Copy link
Member

@tisonkun tisonkun Aug 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, I'd prefer not to set it as a strict rule since a committer should be confident to merge a typo fix directly. Committers share a high trust level. They know when to request one more reviewer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eolivelli @zymap More content can be discussed, I will initiate a discussion, let’s determine the final .asf.yaml
please see the mail https://lists.apache.org/thread/o75xogn8r7zylqlwnno84dsm9nqoh5rs

@hangc0276 hangc0276 added this to the 4.16.0 milestone Oct 14, 2022
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
Descriptions of the changes in this PR:

### Motivation

using [.asf.yaml](https://cwiki.apache.org/confluence/display/INFRA/Git+-+.asf.yaml+features#Git.asf.yamlfeatures-Branchprotection) to configure the project,
but there 's some config bugfix for project's .asf.yaml
the name of workflow's jobs is not set, so apache#3439 causes contexts can't match check's jobs

this picture is the other new add pr's running result: the other pr's workflow run abnormally
<img width="923" alt="image" src="https://user-images.githubusercontent.com/42990025/185718866-b7054dd7-502f-4371-8906-c43f8b1b166b.png">

this picture is current running result: current pr is running normally because the job name is added in yaml file
<img width="954" alt="image" src="https://user-images.githubusercontent.com/42990025/185718710-ec41584e-4871-40cc-9ed6-655caa7ff021.png">
 
### Changes

1.  add the name for checks
2. change required_approving_review_count to 2 person
3. update README.md for workflows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants