amoghrajesh commented on code in PR #36472:
URL: https://github.com/apache/airflow/pull/36472#discussion_r1437793243
##########
CONTRIBUTING.rst:
##########
@@ -453,6 +453,29 @@ Step 4: Prepare PR
PR guidelines described in `pull request guidelines
<#pull-request-guidelines>`_.
Create Pull Request! Make yourself ready for the discussion!
+5. The ``static checks`` and ``tests`` in your PR serve as a
first-line-of-check, whether the PR
+ passes the quality bar for Airflow. It basically means that until you get
your PR green, it is not
+ likely to get reviewed by committers unless you specifically ask for it and
explain that you would like
+ to get first pass of reviews and explain why achieving ``green`` status for
it is not easy/feasible/desired.
+ Similarly if your PR contains ``[WIP]`` in the title or it is marked as
``Draft`` it is not likely to get
+ reviewed by committers unless you specifically ask for it and explain why
and what specifically you want
Review Comment:
We should use the word: `maintainers` here instead
##########
.asf.yaml:
##########
@@ -44,37 +44,47 @@ github:
main:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
+ required_conversation_resolution: true
v1-10-stable:
required_pull_request_reviews:
required_approving_review_count: 1
v2-0-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-1-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-2-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-3-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-4-stable:
required_pull_request_reviews:
required_approving_review_count: 1
+ required_linear_history: true
v2-5-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
+ required_approving_review_count: 1
+ required_linear_history: true
v2-6-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
+ required_approving_review_count: 1
+ required_linear_history: true
v2-7-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
+ required_approving_review_count: 1
+ required_linear_history: true
v2-8-stable:
required_pull_request_reviews:
- required_approving_review_count: 1
-
+ required_approving_review_count: 1
+ required_linear_history: true
collaborators:
- mhenc
- utkarsharma2
Review Comment:
@potiuk unrelated to the PR, but we should remove utkarsh from the
collaborators, right?
##########
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
Review Comment:
```suggestion
It's ok to mark the conversation resolved by anyone who can do it - it
could be the author who thinks
```
##########
CONTRIBUTING.rst:
##########
@@ -453,6 +453,29 @@ Step 4: Prepare PR
PR guidelines described in `pull request guidelines
<#pull-request-guidelines>`_.
Create Pull Request! Make yourself ready for the discussion!
+5. The ``static checks`` and ``tests`` in your PR serve as a
first-line-of-check, whether the PR
+ passes the quality bar for Airflow. It basically means that until you get
your PR green, it is not
+ likely to get reviewed by committers unless you specifically ask for it and
explain that you would like
+ to get first pass of reviews and explain why achieving ``green`` status for
it is not easy/feasible/desired.
+ Similarly if your PR contains ``[WIP]`` in the title or it is marked as
``Draft`` it is not likely to get
+ reviewed by committers unless you specifically ask for it and explain why
and what specifically you want
+ to get reviewed before it reaches ``Ready for review`` status. This might
happen if you want to get initial
+ feedback on the direction of your PR or if you want to get feedback on the
design of your PR.
+
+6. Avoid @-mentioning individual committers in your PR, unless you have good
reason to believe that they are
+ available, have time and interest in your PR. Generally speaking there are
no "exclusive" reviewers for
+ different parts of the code and reviewers review PRs and respond when they
have some free time to spare and
+ when they feel they can provide valuable feedback. If you want to get
attention of committers, you can just
+ follow-up on your PR and ask for review in general, however be considerate
and do not expect "immediate"
+ reviews. People review when they have time, most of the people who are
committers do such reviews in their
+ free time, which is taken away from their families and other interests so
allow sufficient time before you
+ follow-up - but If you see no reaction in several days, do follow-up, as
with the number of PRs we have
Review Comment:
nit: `if` small case
##########
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``
+* no unresolved ``Request changes``
+
+Once you reach the status, you do not need to do anything to get the PR
merged. One of the maintainers
+will merge such PR. However if you see that for a few days such PR is not
merged, do not hesitate to comment
+on your PR and mention that you think it is ready to be merged. Also it's a
good practice to rebase your PR
+to latest ``main``, because there could be other changes merged in the
meantime that might cause conflicts or
+fail tests or static checks, so by rebasing a PR that has been build few days
ago you make sure that it
+still passes the tests and static checks today.
+
+
+.. note:: |experimental|
+
+ In December 2023 we enabled - experimentally - the requirement to resolve
all the open conversations in a
+ PR in order to make it merge-able. You will see in the status of the PR
that it needs to have all the
+ conversations resolved before it can be merged.
+
+ This is an experiment and we will evaluate by the end of January 2024. If
it turns out to be a good idea,
+ we will keep it enabled in the future.
+
+ The goal of this experiment is to make it easier to see when there are some
conversations that are not
+ resolved for everyone involved in the PR - author, reviewers and
maintainers who try to figure out if
+ the PR is ready to merge and ... merge it. The goal is also to use
conversations more as a "soft" way
Review Comment:
The ... is intentional?
##########
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:
```suggestion
* ``approval from 1 (or more for core changes) maintainers``
```
##########
CONTRIBUTING.rst:
##########
@@ -515,14 +647,13 @@ these guidelines:
useful features from newer versions.
- Adhere to guidelines for commit messages described in this `article
<http://chris.beams.io/posts/git-commit/>`__.
- This makes the lives of those who come after you a lot easier.
+ This makes the lives of those who come after you (and your future self) a
lot easier.
Review Comment:
Good addition! 🤣
##########
CONTRIBUTING.rst:
##########
@@ -453,6 +453,29 @@ Step 4: Prepare PR
PR guidelines described in `pull request guidelines
<#pull-request-guidelines>`_.
Create Pull Request! Make yourself ready for the discussion!
+5. The ``static checks`` and ``tests`` in your PR serve as a
first-line-of-check, whether the PR
+ passes the quality bar for Airflow. It basically means that until you get
your PR green, it is not
+ likely to get reviewed by committers unless you specifically ask for it and
explain that you would like
+ to get first pass of reviews and explain why achieving ``green`` status for
it is not easy/feasible/desired.
+ Similarly if your PR contains ``[WIP]`` in the title or it is marked as
``Draft`` it is not likely to get
+ reviewed by committers unless you specifically ask for it and explain why
and what specifically you want
+ to get reviewed before it reaches ``Ready for review`` status. This might
happen if you want to get initial
+ feedback on the direction of your PR or if you want to get feedback on the
design of your PR.
+
+6. Avoid @-mentioning individual committers in your PR, unless you have good
reason to believe that they are
+ available, have time and interest in your PR. Generally speaking there are
no "exclusive" reviewers for
+ different parts of the code and reviewers review PRs and respond when they
have some free time to spare and
+ when they feel they can provide valuable feedback. If you want to get
attention of committers, you can just
+ follow-up on your PR and ask for review in general, however be considerate
and do not expect "immediate"
+ reviews. People review when they have time, most of the people who are
committers do such reviews in their
Review Comment:
nit: Most committers review pull requests
##########
CONTRIBUTING.rst:
##########
@@ -475,38 +586,59 @@ Pull Request Guidelines
Before you submit a pull request (PR) from your forked repo, check that it
meets
these guidelines:
-- Include tests, either as doctests, unit tests, or both, to your pull
- request.
+- Include tests, either as doctests, unit tests, or both, to your pull
request.
The airflow repo uses `GitHub Actions
<https://help.github.com/en/actions>`__ to
run the tests and `codecov <https://codecov.io/gh/apache/airflow>`__ to
track
coverage. You can set up both for free on your fork. It will help you make
sure you do not
break the build with your PR and that you help increase coverage.
+ Also we advise to install locally `pre-commit hooks
<STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__ to
+ apply various checks, code generation and formatting locally at the time
you make a local commit - which
+ gives you near-immediate feedback on things you need to fix before you
push your code to the PR, or in
+ many case it will even fix it for you locally so that you can add and
commit it straight away.
-- Follow our project's `Coding style and best practices`_.
-
- These are things that aren't currently enforced programmatically (either
because they are too hard or just
- not yet done.)
+- Follow our project's `Coding style and best practices`_. Usually we
attempt to enforce the practices by
+ having appropriate pre-commits These are things amongst them that aren't
currently enforced
+ programmatically (either because they are too hard or just not yet done).
-- `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`__, and
resolve all conflicts.
+- We prefer that you ``rebase`` your Pull Request (and do it quite often)
rather than merge. It leads to
+ easier reviews and cleaner changes where you know exactly what changes
you've done. You can learn more
+ about rebase vs. merge workflow in `Rebase and merge your pull request
<https://github.blog/2016-09-26-rebase-and-merge-pull-requests/>`__
+ and `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`__.
Make sure to resolve all conflicts
+ during rebase.
-- When merging PRs, Committer will use **Squash and Merge** which means then
your PR will be merged as one commit, regardless of the number of commits in
your PR. During the review cycle, you can keep a commit history for easier
review, but if you need to, you can also squash all commits to reduce the
maintenance burden during rebase.
+- When merging PRs, Committer will use **Squash and Merge** which means then
your PR will be merged as one
+ commit, regardless of the number of commits in your PR. During the review
cycle, you can keep a commit
+ history for easier review, but if you need to, you can also squash all
commits to reduce the
+ maintenance burden during rebase.
-- Add an `Apache License <http://www.apache.org/legal/src-headers.html>`__
header
- to all new files.
+- Add an `Apache License <http://www.apache.org/legal/src-headers.html>`__
header to all new files (if you
+ have ``pre-commit`` installed, pre-commit will do it automatically for
you. This is one of the reasons
+ why we recommend using ``pre-commit``.
- If you have `pre-commit hooks <STATIC_CODE_CHECKS.rst#pre-commit-hooks>`__
enabled, they automatically add
- license headers during commit.
-
-- If your pull request adds functionality, make sure to update the docs as
part
- of the same PR. Doc string is often sufficient. Make sure to follow the
- Sphinx compatible standards.
+- If your pull request adds functionality, make sure to update the docs as
part of the same PR, not only
+ code and tests. Doc string is often sufficient. Make sure to follow the
Sphinx compatible standards.
Review Comment:
```suggestion
code and tests. Docstring is often sufficient. Make sure to follow the
sphinx compatible standards.
```
##########
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``
+* no unresolved ``Request changes``
+
+Once you reach the status, you do not need to do anything to get the PR
merged. One of the maintainers
+will merge such PR. However if you see that for a few days such PR is not
merged, do not hesitate to comment
+on your PR and mention that you think it is ready to be merged. Also it's a
good practice to rebase your PR
Review Comment:
```suggestion
on your PR and mention that you think it is ready to be merged. Also, it's a
good practice to rebase your PR
```
##########
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``
+* no unresolved ``Request changes``
+
+Once you reach the status, you do not need to do anything to get the PR
merged. One of the maintainers
+will merge such PR. However if you see that for a few days such PR is not
merged, do not hesitate to comment
Review Comment:
```suggestion
will merge such PRs. However if you see that for a few days such a PR is not
merged, do not hesitate to comment
```
--
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
