You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by GitBox <gi...@apache.org> on 2022/11/16 15:41:57 UTC
[GitHub] [incubator-pekko] pjfanning opened a new pull request, #52: require PR reviews and linear history
pjfanning opened a new pull request, #52:
URL: https://github.com/apache/incubator-pekko/pull/52
Based on https://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features
The changes may need tweaking - I won't know if the changes will be applied as expected - so I'll fix any issues where expected updates don't actually get applied
https://github.com/apache/incubator-pekko/issues/51
* requires only 1 PR review - https://lists.apache.org/thread/0hohnn9v0xhxn0qblb196p7mcq7jzz00 and https://cwiki.apache.org/confluence/display/PEKKO/Processes requires 1 review for trivial PRs
* tries to enforce that Scalafmt workflow passes - we can add more later
* linear git history
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025264800
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
> Any review that has open requests or discussion blocks the merge. (only merge when consensus is reached).
To me I think this is too strict, I have been in cases where pull requests stuck open in perpetuity because someone forgets to click the "resolve" button. Either that or committers will just blindly hit resolve which at that point defeats the purpose of it.
Should we discuss in the mailing list? I think for now this is fine, we can always add stricter checks later if there is a consensus on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning closed pull request #52: require PR reviews and linear history
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning closed pull request #52: require PR reviews and linear history
URL: https://github.com/apache/incubator-pekko/pull/52
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025151787
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
Who is in the position to merge pull requests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025264613
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
I don't know how to implement:
```
Any review that has open requests or discussion blocks the merge. (only merge when consensus is reached).
```
If there is a way, it can be implemented.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#issuecomment-1318665319
> Which of these workflows is the 'header check'? Do we need to add a 'header check' workflow first?
its already there, its called `Pull Requests / Check / Headers`,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1024273803
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
So this is an interesting point. Historically akka always required 2 approvals, even for trivial changes. I will say however in terms of practicality especially right now with a lot of mechanical and trivial changes needed to get the project bootstrapped, going with 1 should be fine.
We can revisit this process later, I personally think that requiring 2 reviewers once the project is setup has merit especially since at that point in time we will also start seeing non trivial functional changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] Claudenw commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025339240
##########
.asf.yaml:
##########
@@ -28,7 +28,22 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
Review Comment:
Please add:
```## Licensed under the terms of http://www.apache.org/licenses/LICENSE-2.0```
To the top of the file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1064103774
##########
.asf.yaml:
##########
@@ -1,5 +1,20 @@
-# https://cwiki.apache.org/confluence/display/INFRA/.asf.yaml+features+for+git+repositories
+# Licensed to the Apache Software Foundation (ASF) under one or more
Review Comment:
@pjfanning I think you can remove this header change because its going to be done properly in https://github.com/apache/incubator-pekko/pull/50
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #52: require PR reviews and linear history
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#issuecomment-1422343817
I'll close this as it is stale
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025163254
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
anyone with git commit rights - all Pekko committers can do this - you might need to link your Github and ASF accounts if you haven't done this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1099885689
##########
.asf.yaml:
##########
@@ -28,7 +43,22 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+ - Pull Requests / Check / Headers
Review Comment:
I would recommend checking that this works separately, as per https://github.com/apache/incubator-pekko-sbt-paradox/pull/5#issuecomment-1414453790 otherwise we can freeze the main branch and we have to make an INFRA ticket to unfreeze it.
The way I did this I pushed a new branch (ergo `test`), created a PR against `main` that only enables the `contexts` check against `test` and then make another PR against test to figure out if the check is satisfied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] Claudenw commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025425654
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
@pjfanning I think that
``` required_conversation_resolution: true```
will require the conversation resolution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1024228478
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
only 1 review needed for trivial changes according to https://cwiki.apache.org/confluence/display/PEKKO/Processes - there is no way to work out automatically what is trivial
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1099883068
##########
.asf.yaml:
##########
@@ -28,7 +43,22 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
Review Comment:
As per https://github.com/apache/incubator-pekko-sbt-paradox/pull/7, this needs to be changed to `- Code is formatted` rather than `- Scalafmt`
##########
.asf.yaml:
##########
@@ -28,7 +43,22 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+ - Pull Requests / Check / Headers
Review Comment:
I would recommend checking that this works in a separately. As per https://github.com/apache/incubator-pekko-sbt-paradox/pull/5#issuecomment-1414453790 otherwise we can freeze the main branch and we have to make an INFRA ticket to unfreeze it. The way I did this I pushed a new branch (ergo `test`), created a PR against `main` that only enables the `contexts` check against `test` and then make another PR against test to figure out if the check is satisfied.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025174572
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
So the origin author and who proved the PR can merge it too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
He-Pin commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1024199084
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
at least 2?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#issuecomment-1318608885
> Is it possible to add the header check in conjunction with scalafmt?
Which of these workflows is the 'header check'? Do we need to add a 'header check' workflow first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025572004
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
I don't see `required_conversation_resolution` in https://cwiki.apache.org/confluence/display/INFRA/git+-+.asf.yaml+features
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] Claudenw commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025260449
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
I was looking for a couple of things to be addressed:
1. At least one review
2. Any review that has open requests or discussion blocks the merge. (only merge when consensus is reached).
I don't have an opinion on scalafmt in particular, but I believe that there should be checks for formatting in general.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1024273803
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
So this is an interesting point. Historically akka always required 2 approvals, even for trivial changes. I will say however in terms of practicality especially right now with a lot of mechanical and trivial changes needed to her the project going 1 should be fine.
We can revisit this process later, I personally think that requiring 2 reviewers once the project is setup has merit especially since at that point in time we will also start seeing non trivial functional changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025264800
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
> Any review that has open requests or discussion blocks the merge. (only merge when consensus is reached).
To me I think this is too strict, I have been in cases where pull requests stuck open in perpetuity because someone forgets to click the "resolve" button. Either that or committers will just blindly hit resolve which at that point defeats the purpose of it.
Should we discuss in the mailing list? I think for now this is fine, we can always add stricter checks later if there is a consensus on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025177291
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
I guess the original PR author can merge. I, personally, see no issue with this - so long as the reviews have been done. No discussion has made in the team that suggests that anyone has any problem with this either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
mdedetrich commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025279992
##########
.asf.yaml:
##########
@@ -28,7 +28,21 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
+ 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:
+ - Scalafmt
+
+ required_pull_request_reviews:
+ dismiss_stale_reviews: false
+ require_code_owner_reviews: false
+ required_approving_review_count: 1
Review Comment:
I would say lets just just merge the PR as it is now and we can also make a future one to implement these stricter checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on a diff in pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on code in PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#discussion_r1025573591
##########
.asf.yaml:
##########
@@ -28,7 +28,22 @@ github:
rebase: true
protected_branches:
- main: { }
+ main:
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] pjfanning commented on pull request #52: require PR reviews and linear history
Posted by GitBox <gi...@apache.org>.
pjfanning commented on PR #52:
URL: https://github.com/apache/incubator-pekko/pull/52#issuecomment-1327800970
@Claudenw Is it possible for you to re-review this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org