potiuk commented on code in PR #36472:
URL: https://github.com/apache/airflow/pull/36472#discussion_r1438111964
##########
CONTRIBUTING.rst:
##########
@@ -461,52 +494,159 @@ Step 5: Pass PR Review
:align: center
:alt: PR Review
-Note that committers will use **Squash and Merge** instead of **Rebase and
Merge**
+Note that maintainers 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
-another committer). Ideally you should have 2 or more committers reviewing the
code that touches
-the core of Airflow.
-
-
-Pull Request Guidelines
+When a reviewer starts a conversation 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. The whole idea of a
conversation is try to reach a consensus
+ on a good way to address the problem. As an author you can resolve the
conversation if you think the
+ problem raised in the comment is resolved or ask the reviewer to re-review,
confirm If you do not understand
+ the comment, you can ask for clarifications. Generally assume good intention
of the person who is reviewing
+ your code and resolve conversations also having good intentions. Understand
that it's not a person that
+ is criticised or argued with, but rather the code and the approach. The
important thing is to take care
+ about quality of the the code and the project and want to make sure that the
code is good
+
+ It's ok to mark the conversation resolved by anyone who can do it - it could
be the author, who 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 maintainers 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 maintainer (if you are
maintainer yourself, it has to be
+another maintainer). Ideally you should have 2 or more maintainers reviewing
the code that touches
+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 a 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 PRs. However if you see that for a few days such a 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 - eventually - merge it. The goal is also to
use conversations more as a "soft" way
+ to request changes and limit the use of ``Request changes`` status to only
those cases when the maintainer
+ is sure that the PR should not be merged in the current state. That should
lead to faster review/merge
+ cycle and less problems with stalled PRs that have ``Request changes``
status but all the issues are
+ already solved (assuming that maintainers will start treating the
conversations this way).
+
+
+Pull Request guidelines
=======================
-Before you submit a pull request (PR) from your forked repo, check that it
meets
+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 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`_.
+- Follow our project's `Coding style and best practices`_. Usually we
attempt to enforce the practices by
+ having appropriate pre-commits. There are checks amongst them that aren't
currently enforced
+ programmatically (either because they are too hard or just not yet done).
- These are things that aren't currently enforced programmatically (either
because they are too hard or just
- not yet done.)
+- We prefer that you ``rebase`` your PR (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.
-- `Rebase your fork <http://stackoverflow.com/a/7244456/1110993>`__, and
resolve all conflicts.
+- When merging PRs, Maintainer 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. 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``.
Review Comment:
I reworded it. My main point is that `pre-commit` saves you from all those
kind of mundane and repetitive things that people are wining about. For me
`pre-commit` adding licence headers is THE huge win that justifies the little
slow-down when committing.
--
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
