potiuk commented on code in PR #36472: URL: https://github.com/apache/airflow/pull/36472#discussion_r1437834726
########## CONTRIBUTING.rst: ########## @@ -464,9 +487,97 @@ Step 5: Pass PR Review Note that committers will use **Squash and Merge** instead of **Rebase and Merge** when merging PRs and your commit will be squashed to single commit. -You need to have review of at least one committer (if you are committer yourself, it has to be +When the maintainer starts conversations it is expected that you respond to questions, suggestions, doubts, +and generally it's great if all such conversations seem to converge to a common understanding. You do not +necessarily have to apply all the suggestions (often they are just opinions and suggestions even if they are +coming from seasoned maintainers) - it's perfectly ok that you respond to it with your own opinions and +understanding of the problem and your approach and if you have good arguments, presenting them is a good idea. + +The reviewers might leave several types of responses: + +* ``General PR comment`` - which usually means that there is a question/opinion/suggestion on how the PR can be + improved, or it's an ask to explain how you understand the PR. You can usually quote some parts of such + general comment and respond to it in your comments. Often comments that are raising questions in general + might lead to different discussions, even a request to move the discussion to the devlist or even lead to + completely new PRs created as a spin-off of the discussion. + +* ``Comment/Conversation around specific lines of code`` - such conversation usually flags a potential + improvement, or a potential problem with the code. It's a good idea to respond to such comments and explain + your approach and understanding of the problem. If you do not agree with the comment, you can explain why + you think your approach is better. If you agree with the comment, you can explain why you did not do it + this way in the first place. If you agree with the comment and you think it's a good idea, you can just + apply the suggestion and push the change to your PR. You can also resolve the conversation if you think the + problem raised in the comment or ask the reviewer to re-review, confirm it and resolve the conversation if + you are not sure. If you do not understand the comment, you can ask for clarifications. Generally assume + good intention of the person who is reviewing your code, they are not criticising you, at most they care + about the quality of the code and the project and want to make sure that the code is as good as possible. + + It's ok to mark the conversation resolved by anyone who can do it - it could be the author, that thinks + the arguments are changes implemented make the conversation resolved, or the maintainer/person who + started the conversation or it can be even marked as resolved by the maintainer who attempts to merge the + PR and thinks that all conversations are resolved. However if you want to make sure attention and decision + on merging the PR is given by maintainer, make sure you monitor, follow-up and close the conversations when + you think they are resolved (ideally explaining why you think the conversation is resolved). + +* ``Request changes`` - this is where maintainer is pretty sure that you should make a change to your PR + because it contains serious flaw, design misconception, or a bug or it is just not in-line with the common + approach Airflow community took on the issue. Usually you should respond to such request and either fix + the problem or convince the maintainer that they were wrong (it happens more often than you think). + Sometimes even if you do not agree with the request, it's a good idea to make the change anyway, because + it might be a good idea to follow the common approach in the project. Sometimes it might even happen that + two maintainer will have completely different opinions on the same issue and you will have to lead the + discussion to try to achieve consensus. If you cannot achieve consensus and you think it's an important + issue, you can ask for a vote on the issue by raising a devlist discussion - where you explain your case + and follow up the discussion with a vote when you cannot achieve consensus there. The ``Request changes`` + status can be withdrawn by the maintainer, but if they don't - such PR cannot be merged - maintainers have + the right to veto any code modification according to the `Apache Software Foundation rules <https://www.apache.org/foundation/voting.html#votes-on-code-modification>`_. + +* ``Approval`` - this is given by a maintainer after the code has been reviewed and the maintainer agrees that + it is a good idea to merge it. There might still be some unresolved conversations, requests and questions on + such PR and you are expected to resolve them before the PR is merged. But the ``Approval`` status is a sign + of trust from the maintainer who gave the approval that they think the PR is good enough as long as their + comments will be resolved and they put the trust in the hands of the author and - possibly - other + maintainers who will merge the request that they can do that without follow-up re-review and verification. + + +You need to have ``Approval`` of at least one committer (if you are committer yourself, it has to be another committer). Ideally you should have 2 or more committers reviewing the code that touches -the core of Airflow. +the core of Airflow - we do not have enforcement about ``2+`` reviewers required for Core of Airflow, +but Maintainers will generally ask in the PR if they think second review is needed. + +Your PR can be merged by the reviewer or another maintainer who will see that the +PR is approved, all conversations are resolved and the code looks good. The criteria for PR +being merge-able are: + +* ``green status for static checks and tests`` +* ``conversations resolved`` +* ``Approval from 1 (or more for core changes) maintainers`` Review Comment: yep -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
