Reminder to use draft mode

[Owen Nichols <on...@vmware.com>]

Please keep your PR in draft mode anytime it is not ready to be reviewed.

This includes if you have received request for changes, or if any PR checks are not passing.

If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

This small step will help us respect one anothers’ time and focus.  Thanks.

Re: Reminder to use draft mode

[Mark Hanson <ha...@vmware.com>]

No, I was not aware, so I think people will have to remember for themselves. Still, I think this is minor. Thank you for the info!

On 5/19/21, 4:28 AM, "Alberto Bustamante Reyes" <al...@est.tech> wrote:

    Most probably you are already aware of it (and this is why it has not been changed in geode repo) but it seems github does not allow to configure draft PRs as default option for a given repository.



    ________________________________
    De: Mark Hanson <ha...@vmware.com>
    Enviado: viernes, 7 de mayo de 2021 23:06
    Para: dev@geode.apache.org <de...@geode.apache.org>; Blake Bender <bb...@vmware.com>
    Asunto: Re: Reminder to use draft mode

    Correct me if I am wrong, but I think the basic consensus here is that the starting state for all PRs should be draft. We should change the default if we can... The sticky part is when to put a PR from standard to draft. If we always start with draft mode, lets deal with that when it becomes a real problem. I don't think it will be a significant one if the consensus is we start with draft PRs.

    Thanks,
    Mark

    On 5/7/21, 2:03 PM, "Mark Hanson" <ha...@vmware.com> wrote:

        @Blake Bender The same goes for the geode code. The PR pipeline is *the* way to know if we broke something or not. Most people don't know how to run the individual tests.

        Thanks,
        Mark

        On 5/7/21, 10:07 AM, "Blake Bender" <bb...@vmware.com> wrote:

            +1 for draft mode as default.  I'm forever switching to it in geode-native already, because the most convenient way for us to get feedback on build/test status for all platforms is to run a change through CI, and the only way to do that is to submit it as a PR.

            Thanks,

            Blake


            -----Original Message-----
            From: Alberto Bustamante Reyes <al...@est.tech>
            Sent: Thursday, May 6, 2021 1:19 PM
            To: dev@geode.apache.org
            Subject: RE: Reminder to use draft mode

            +1 to Mark's proposal of setting draft mode as default when creating PRs (Im wondering if a new VOTE thread is needed to approve it)

            And also +1 to Donal's comments.

            ________________________________
            De: Darrel Schneider <da...@vmware.com>
            Enviado: jueves, 6 de mayo de 2021 21:43
            Para: dev@geode.apache.org <de...@geode.apache.org>
            Asunto: Re: Reminder to use draft mode

            +1 to Donal's comments
            ________________________________
            From: Donal Evans <do...@vmware.com>
            Sent: Thursday, May 6, 2021 11:44 AM
            To: dev@geode.apache.org <de...@geode.apache.org>
            Subject: Re: Reminder to use draft mode

            +1 to Naba's PR flow described above.

            Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

            However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

            Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

            For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

            Donal
            ________________________________
            From: Nabarun Nag <nn...@vmware.com>
            Sent: Thursday, May 6, 2021 10:22 AM
            To: dev@geode.apache.org <de...@geode.apache.org>
            Subject: Re: Reminder to use draft mode

            I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

            There has been this situation where,

              *   PR is created (reviewers are assigned)
              *   approved
              *   Tests fail
              *   code is changed
              *   no reviews
              *   code is merged

            Hence code that is not reviewed has been merged

            This way of doing work also has the following advantages:

              *   A reviewer does not have to review a code that causes tests to fail
              *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
              *   Unreviewed code post-test fixes do not get merged

            I think this way of working saves a critical amount of time for engineers who review code.

            This flow of PRs feels more efficient:


              *   Create PR in draft mode - no reviewers assigned
              *   PRechecks fail
              *   change/fix code
              *   tests pass - all green
              *   convert PR to ready for review - reviewers assigned
              *   reviewers review

            Regards
            Naba



            ________________________________
            From: Owen Nichols <on...@vmware.com>
            Sent: Thursday, May 6, 2021 9:59 AM
            To: dev@geode.apache.org <de...@geode.apache.org>
            Subject: Re: Reminder to use draft mode

            Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

            On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

                I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

                I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

                Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

                Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

                --Jens

                From: Owen Nichols <on...@vmware.com>
                Date: Thursday, May 6, 2021 at 9:12 AM
                To: dev@geode.apache.org <de...@geode.apache.org>
                Subject: Re: Reminder to use draft mode
                A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

                Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

                I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

                Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

                On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

                    Comments inline…

                    Please keep your PR in draft mode anytime it is not ready to be reviewed.

                    This includes if you have received request for changes, or if any PR checks are not passing.

                    How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

                    Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


                    If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

                    I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

                    --Jens

RE: Reminder to use draft mode

[Alberto Bustamante Reyes <al...@est.tech>]

Most probably you are already aware of it (and this is why it has not been changed in geode repo) but it seems github does not allow to configure draft PRs as default option for a given repository.



________________________________
De: Mark Hanson <ha...@vmware.com>
Enviado: viernes, 7 de mayo de 2021 23:06
Para: dev@geode.apache.org <de...@geode.apache.org>; Blake Bender <bb...@vmware.com>
Asunto: Re: Reminder to use draft mode

Correct me if I am wrong, but I think the basic consensus here is that the starting state for all PRs should be draft. We should change the default if we can... The sticky part is when to put a PR from standard to draft. If we always start with draft mode, lets deal with that when it becomes a real problem. I don't think it will be a significant one if the consensus is we start with draft PRs.

Thanks,
Mark

On 5/7/21, 2:03 PM, "Mark Hanson" <ha...@vmware.com> wrote:

    @Blake Bender The same goes for the geode code. The PR pipeline is *the* way to know if we broke something or not. Most people don't know how to run the individual tests.

    Thanks,
    Mark

    On 5/7/21, 10:07 AM, "Blake Bender" <bb...@vmware.com> wrote:

        +1 for draft mode as default.  I'm forever switching to it in geode-native already, because the most convenient way for us to get feedback on build/test status for all platforms is to run a change through CI, and the only way to do that is to submit it as a PR.

        Thanks,

        Blake


        -----Original Message-----
        From: Alberto Bustamante Reyes <al...@est.tech>
        Sent: Thursday, May 6, 2021 1:19 PM
        To: dev@geode.apache.org
        Subject: RE: Reminder to use draft mode

        +1 to Mark's proposal of setting draft mode as default when creating PRs (Im wondering if a new VOTE thread is needed to approve it)

        And also +1 to Donal's comments.

        ________________________________
        De: Darrel Schneider <da...@vmware.com>
        Enviado: jueves, 6 de mayo de 2021 21:43
        Para: dev@geode.apache.org <de...@geode.apache.org>
        Asunto: Re: Reminder to use draft mode

        +1 to Donal's comments
        ________________________________
        From: Donal Evans <do...@vmware.com>
        Sent: Thursday, May 6, 2021 11:44 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode

        +1 to Naba's PR flow described above.

        Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

        However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

        Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

        For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

        Donal
        ________________________________
        From: Nabarun Nag <nn...@vmware.com>
        Sent: Thursday, May 6, 2021 10:22 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode

        I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

        There has been this situation where,

          *   PR is created (reviewers are assigned)
          *   approved
          *   Tests fail
          *   code is changed
          *   no reviews
          *   code is merged

        Hence code that is not reviewed has been merged

        This way of doing work also has the following advantages:

          *   A reviewer does not have to review a code that causes tests to fail
          *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
          *   Unreviewed code post-test fixes do not get merged

        I think this way of working saves a critical amount of time for engineers who review code.

        This flow of PRs feels more efficient:


          *   Create PR in draft mode - no reviewers assigned
          *   PRechecks fail
          *   change/fix code
          *   tests pass - all green
          *   convert PR to ready for review - reviewers assigned
          *   reviewers review

        Regards
        Naba



        ________________________________
        From: Owen Nichols <on...@vmware.com>
        Sent: Thursday, May 6, 2021 9:59 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode

        Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

        On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

            I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

            I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

            Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

            Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

            --Jens

            From: Owen Nichols <on...@vmware.com>
            Date: Thursday, May 6, 2021 at 9:12 AM
            To: dev@geode.apache.org <de...@geode.apache.org>
            Subject: Re: Reminder to use draft mode
            A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

            Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

            I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

            Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

            On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

                Comments inline…

                Please keep your PR in draft mode anytime it is not ready to be reviewed.

                This includes if you have received request for changes, or if any PR checks are not passing.

                How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

                Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


                If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

                I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

                --Jens

Re: Reminder to use draft mode

[Mark Hanson <ha...@vmware.com>]

Correct me if I am wrong, but I think the basic consensus here is that the starting state for all PRs should be draft. We should change the default if we can... The sticky part is when to put a PR from standard to draft. If we always start with draft mode, lets deal with that when it becomes a real problem. I don't think it will be a significant one if the consensus is we start with draft PRs.

Thanks,
Mark

On 5/7/21, 2:03 PM, "Mark Hanson" <ha...@vmware.com> wrote:

    @Blake Bender The same goes for the geode code. The PR pipeline is *the* way to know if we broke something or not. Most people don't know how to run the individual tests.

    Thanks,
    Mark

    On 5/7/21, 10:07 AM, "Blake Bender" <bb...@vmware.com> wrote:

        +1 for draft mode as default.  I'm forever switching to it in geode-native already, because the most convenient way for us to get feedback on build/test status for all platforms is to run a change through CI, and the only way to do that is to submit it as a PR.

        Thanks,

        Blake


        -----Original Message-----
        From: Alberto Bustamante Reyes <al...@est.tech> 
        Sent: Thursday, May 6, 2021 1:19 PM
        To: dev@geode.apache.org
        Subject: RE: Reminder to use draft mode

        +1 to Mark's proposal of setting draft mode as default when creating PRs (Im wondering if a new VOTE thread is needed to approve it)

        And also +1 to Donal's comments.

        ________________________________
        De: Darrel Schneider <da...@vmware.com>
        Enviado: jueves, 6 de mayo de 2021 21:43
        Para: dev@geode.apache.org <de...@geode.apache.org>
        Asunto: Re: Reminder to use draft mode

        +1 to Donal's comments
        ________________________________
        From: Donal Evans <do...@vmware.com>
        Sent: Thursday, May 6, 2021 11:44 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode

        +1 to Naba's PR flow described above.

        Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

        However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

        Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

        For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

        Donal
        ________________________________
        From: Nabarun Nag <nn...@vmware.com>
        Sent: Thursday, May 6, 2021 10:22 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode

        I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

        There has been this situation where,

          *   PR is created (reviewers are assigned)
          *   approved
          *   Tests fail
          *   code is changed
          *   no reviews
          *   code is merged

        Hence code that is not reviewed has been merged

        This way of doing work also has the following advantages:

          *   A reviewer does not have to review a code that causes tests to fail
          *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
          *   Unreviewed code post-test fixes do not get merged

        I think this way of working saves a critical amount of time for engineers who review code.

        This flow of PRs feels more efficient:


          *   Create PR in draft mode - no reviewers assigned
          *   PRechecks fail
          *   change/fix code
          *   tests pass - all green
          *   convert PR to ready for review - reviewers assigned
          *   reviewers review

        Regards
        Naba



        ________________________________
        From: Owen Nichols <on...@vmware.com>
        Sent: Thursday, May 6, 2021 9:59 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode

        Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

        On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

            I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

            I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

            Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

            Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

            --Jens

            From: Owen Nichols <on...@vmware.com>
            Date: Thursday, May 6, 2021 at 9:12 AM
            To: dev@geode.apache.org <de...@geode.apache.org>
            Subject: Re: Reminder to use draft mode
            A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

            Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

            I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

            Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

            On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

                Comments inline…

                Please keep your PR in draft mode anytime it is not ready to be reviewed.

                This includes if you have received request for changes, or if any PR checks are not passing.

                How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

                Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


                If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

                I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

                --Jens

Re: Reminder to use draft mode

[Mark Hanson <ha...@vmware.com>]

@Blake Bender The same goes for the geode code. The PR pipeline is *the* way to know if we broke something or not. Most people don't know how to run the individual tests.

Thanks,
Mark

On 5/7/21, 10:07 AM, "Blake Bender" <bb...@vmware.com> wrote:

    +1 for draft mode as default.  I'm forever switching to it in geode-native already, because the most convenient way for us to get feedback on build/test status for all platforms is to run a change through CI, and the only way to do that is to submit it as a PR.

    Thanks,

    Blake


    -----Original Message-----
    From: Alberto Bustamante Reyes <al...@est.tech> 
    Sent: Thursday, May 6, 2021 1:19 PM
    To: dev@geode.apache.org
    Subject: RE: Reminder to use draft mode

    +1 to Mark's proposal of setting draft mode as default when creating PRs (Im wondering if a new VOTE thread is needed to approve it)

    And also +1 to Donal's comments.

    ________________________________
    De: Darrel Schneider <da...@vmware.com>
    Enviado: jueves, 6 de mayo de 2021 21:43
    Para: dev@geode.apache.org <de...@geode.apache.org>
    Asunto: Re: Reminder to use draft mode

    +1 to Donal's comments
    ________________________________
    From: Donal Evans <do...@vmware.com>
    Sent: Thursday, May 6, 2021 11:44 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode

    +1 to Naba's PR flow described above.

    Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

    However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

    Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

    For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

    Donal
    ________________________________
    From: Nabarun Nag <nn...@vmware.com>
    Sent: Thursday, May 6, 2021 10:22 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode

    I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

    There has been this situation where,

      *   PR is created (reviewers are assigned)
      *   approved
      *   Tests fail
      *   code is changed
      *   no reviews
      *   code is merged

    Hence code that is not reviewed has been merged

    This way of doing work also has the following advantages:

      *   A reviewer does not have to review a code that causes tests to fail
      *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
      *   Unreviewed code post-test fixes do not get merged

    I think this way of working saves a critical amount of time for engineers who review code.

    This flow of PRs feels more efficient:


      *   Create PR in draft mode - no reviewers assigned
      *   PRechecks fail
      *   change/fix code
      *   tests pass - all green
      *   convert PR to ready for review - reviewers assigned
      *   reviewers review

    Regards
    Naba



    ________________________________
    From: Owen Nichols <on...@vmware.com>
    Sent: Thursday, May 6, 2021 9:59 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode

    Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

    On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

        I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

        Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

        Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

        --Jens

        From: Owen Nichols <on...@vmware.com>
        Date: Thursday, May 6, 2021 at 9:12 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode
        A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

        Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

        I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

        Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

        On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

            Comments inline…

            Please keep your PR in draft mode anytime it is not ready to be reviewed.

            This includes if you have received request for changes, or if any PR checks are not passing.

            How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

            Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


            If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

            I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

            --Jens

RE: Reminder to use draft mode

[Blake Bender <bb...@vmware.com>]

+1 for draft mode as default.  I'm forever switching to it in geode-native already, because the most convenient way for us to get feedback on build/test status for all platforms is to run a change through CI, and the only way to do that is to submit it as a PR.

Thanks,

Blake


-----Original Message-----
From: Alberto Bustamante Reyes <al...@est.tech> 
Sent: Thursday, May 6, 2021 1:19 PM
To: dev@geode.apache.org
Subject: RE: Reminder to use draft mode

+1 to Mark's proposal of setting draft mode as default when creating PRs (Im wondering if a new VOTE thread is needed to approve it)

And also +1 to Donal's comments.

________________________________
De: Darrel Schneider <da...@vmware.com>
Enviado: jueves, 6 de mayo de 2021 21:43
Para: dev@geode.apache.org <de...@geode.apache.org>
Asunto: Re: Reminder to use draft mode

+1 to Donal's comments
________________________________
From: Donal Evans <do...@vmware.com>
Sent: Thursday, May 6, 2021 11:44 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

+1 to Naba's PR flow described above.

Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

Donal
________________________________
From: Nabarun Nag <nn...@vmware.com>
Sent: Thursday, May 6, 2021 10:22 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

There has been this situation where,

  *   PR is created (reviewers are assigned)
  *   approved
  *   Tests fail
  *   code is changed
  *   no reviews
  *   code is merged

Hence code that is not reviewed has been merged

This way of doing work also has the following advantages:

  *   A reviewer does not have to review a code that causes tests to fail
  *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
  *   Unreviewed code post-test fixes do not get merged

I think this way of working saves a critical amount of time for engineers who review code.

This flow of PRs feels more efficient:


  *   Create PR in draft mode - no reviewers assigned
  *   PRechecks fail
  *   change/fix code
  *   tests pass - all green
  *   convert PR to ready for review - reviewers assigned
  *   reviewers review

Regards
Naba



________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Thursday, May 6, 2021 9:59 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

    I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

    Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

    Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

    --Jens

    From: Owen Nichols <on...@vmware.com>
    Date: Thursday, May 6, 2021 at 9:12 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode
    A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

    Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

    I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

    Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

    On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        Comments inline…

        Please keep your PR in draft mode anytime it is not ready to be reviewed.

        This includes if you have received request for changes, or if any PR checks are not passing.

        How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

        Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


        If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

        I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

        --Jens

Re: Reminder to use draft mode

[Anthony Baker <ba...@vmware.com>]

Side note:  I think using discussion to achieve consensus on topics like this tends to work better than [VOTE] threads.  If we fail to reach a consensus we can resort to a vote thread, or for reasons spelled out in [1].

IMHO,
Anthony

[1] https://www.apache.org/foundation/voting.html


On May 6, 2021, at 1:18 PM, Alberto Bustamante Reyes <al...@est.tech>> wrote:

 (Im wondering if a new VOTE thread is needed to approve it)

RE: Reminder to use draft mode

[Alberto Bustamante Reyes <al...@est.tech>]

+1 to Mark's proposal of setting draft mode as default when creating PRs (Im wondering if a new VOTE thread is needed to approve it)

And also +1 to Donal's comments.

________________________________
De: Darrel Schneider <da...@vmware.com>
Enviado: jueves, 6 de mayo de 2021 21:43
Para: dev@geode.apache.org <de...@geode.apache.org>
Asunto: Re: Reminder to use draft mode

+1 to Donal's comments
________________________________
From: Donal Evans <do...@vmware.com>
Sent: Thursday, May 6, 2021 11:44 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

+1 to Naba's PR flow described above.

Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

Donal
________________________________
From: Nabarun Nag <nn...@vmware.com>
Sent: Thursday, May 6, 2021 10:22 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

There has been this situation where,

  *   PR is created (reviewers are assigned)
  *   approved
  *   Tests fail
  *   code is changed
  *   no reviews
  *   code is merged

Hence code that is not reviewed has been merged

This way of doing work also has the following advantages:

  *   A reviewer does not have to review a code that causes tests to fail
  *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
  *   Unreviewed code post-test fixes do not get merged

I think this way of working saves a critical amount of time for engineers who review code.

This flow of PRs feels more efficient:


  *   Create PR in draft mode - no reviewers assigned
  *   PRechecks fail
  *   change/fix code
  *   tests pass - all green
  *   convert PR to ready for review - reviewers assigned
  *   reviewers review

Regards
Naba



________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Thursday, May 6, 2021 9:59 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

    I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

    Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

    Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

    --Jens

    From: Owen Nichols <on...@vmware.com>
    Date: Thursday, May 6, 2021 at 9:12 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode
    A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

    Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

    I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

    Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

    On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        Comments inline…

        Please keep your PR in draft mode anytime it is not ready to be reviewed.

        This includes if you have received request for changes, or if any PR checks are not passing.

        How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

        Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


        If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

        I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

        --Jens

Re: Reminder to use draft mode

[Darrel Schneider <da...@vmware.com>]

+1 to Donal's comments
________________________________
From: Donal Evans <do...@vmware.com>
Sent: Thursday, May 6, 2021 11:44 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

+1 to Naba's PR flow described above.

Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

Donal
________________________________
From: Nabarun Nag <nn...@vmware.com>
Sent: Thursday, May 6, 2021 10:22 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

There has been this situation where,

  *   PR is created (reviewers are assigned)
  *   approved
  *   Tests fail
  *   code is changed
  *   no reviews
  *   code is merged

Hence code that is not reviewed has been merged

This way of doing work also has the following advantages:

  *   A reviewer does not have to review a code that causes tests to fail
  *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
  *   Unreviewed code post-test fixes do not get merged

I think this way of working saves a critical amount of time for engineers who review code.

This flow of PRs feels more efficient:


  *   Create PR in draft mode - no reviewers assigned
  *   PRechecks fail
  *   change/fix code
  *   tests pass - all green
  *   convert PR to ready for review - reviewers assigned
  *   reviewers review

Regards
Naba



________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Thursday, May 6, 2021 9:59 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

    I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

    Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

    Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

    --Jens

    From: Owen Nichols <on...@vmware.com>
    Date: Thursday, May 6, 2021 at 9:12 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode
    A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

    Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

    I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

    Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

    On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        Comments inline…

        Please keep your PR in draft mode anytime it is not ready to be reviewed.

        This includes if you have received request for changes, or if any PR checks are not passing.

        How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

        Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


        If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

        I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

        --Jens

Re: Reminder to use draft mode

[Donal Evans <do...@vmware.com>]

+1 to Naba's PR flow described above.

Creating PRs in draft mode is almost always the best choice, as it prevents people from being tagged to review a set of changes that may change significantly due to test failures and only requires a single click to convert to the "ready to review" state - hardly a major inconvenience.

However, the real tricky question here seems to be "When should you move a PR from "Ready to review" back into draft mode?" I tend to agree with Jens that a flaky test failure by itself isn't enough to warrant putting a PR back into draft mode, as it's often possible to identify the failure as being due to an existing known bug and merge the PR knowing that your changes aren't the cause. We don't require that all PR tests are green before merging, just some of them, so it's reasonable to assume that we don't require all PR tests to be green before a PR is considered ready for review either.

Minor edits due to review comments (like spelling mistakes or minor code quality/style changes) also don't feel like they should cause a PR to be put back into draft mode, as while the contents of the PR may change because of them, it won't invalidate other in-progress reviews if it does, or significantly alter the nature of the PR.

For me, the bar for whether a PR should be put back into draft mode is if you know that its current state is not reflective of the final state that will be merged into develop. In general, the only time that should happen is if you've received review feedback that will require a change of approach or significant refactoring/additional code. It's the difference between "needs a little polish" and "needs more work," I think. Obviously, what counts as "significant" is entirely subjective, so this isn't much use as a hard and fast rule, but a rough guide might be that if a reviewer has requested changes that would invalidate or render obsolete/redundant any additional reviews that come in before those changes are applied, moving back to draft mode would probably be a good idea.

Donal
________________________________
From: Nabarun Nag <nn...@vmware.com>
Sent: Thursday, May 6, 2021 10:22 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

There has been this situation where,

  *   PR is created (reviewers are assigned)
  *   approved
  *   Tests fail
  *   code is changed
  *   no reviews
  *   code is merged

Hence code that is not reviewed has been merged

This way of doing work also has the following advantages:

  *   A reviewer does not have to review a code that causes tests to fail
  *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
  *   Unreviewed code post-test fixes do not get merged

I think this way of working saves a critical amount of time for engineers who review code.

This flow of PRs feels more efficient:


  *   Create PR in draft mode - no reviewers assigned
  *   PRechecks fail
  *   change/fix code
  *   tests pass - all green
  *   convert PR to ready for review - reviewers assigned
  *   reviewers review

Regards
Naba



________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Thursday, May 6, 2021 9:59 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

    I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

    Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

    Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

    --Jens

    From: Owen Nichols <on...@vmware.com>
    Date: Thursday, May 6, 2021 at 9:12 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode
    A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

    Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

    I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

    Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

    On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        Comments inline…

        Please keep your PR in draft mode anytime it is not ready to be reviewed.

        This includes if you have received request for changes, or if any PR checks are not passing.

        How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

        Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


        If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

        I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

        --Jens

Re: Reminder to use draft mode

[Jens Deppe <jd...@vmware.com>]

To be clear. I’m absolutely in favor of using draft mode as an initial indicator of the state of a PR.

What I’m not in favor of is requiring the PR to be switched back and forth. Certainly, if any individual developer wants to do that, of course that’s their prerogative.

--Jens.

From: Mark Hanson <ha...@vmware.com>
Date: Thursday, May 6, 2021 at 10:45 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode
I agree, I like the draft mode switch. The hesitations that I have are mentioned by Jens in that you can have failures that are unrelated. Especially DUnits at this point. Perhaps for required tests following the draft mode approach is better. I have had many cases where I see PRs that obviously need work asking for my review and I have to look past them in my review requested PR list. There have been several PRs that have been untouched for months that were failing tests but my review was required...

+1 for draft mode as the best approach to get to passing tests, then normal mode once the PR is ready. (not that we are voting)

Thanks,
Mark

On 5/6/21, 10:22 AM, "Nabarun Nag" <nn...@vmware.com> wrote:

    I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

    There has been this situation where,

      *   PR is created (reviewers are assigned)
      *   approved
      *   Tests fail
      *   code is changed
      *   no reviews
      *   code is merged

    Hence code that is not reviewed has been merged

    This way of doing work also has the following advantages:

      *   A reviewer does not have to review a code that causes tests to fail
      *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
      *   Unreviewed code post-test fixes do not get merged

    I think this way of working saves a critical amount of time for engineers who review code.

    This flow of PRs feels more efficient:


      *   Create PR in draft mode - no reviewers assigned
      *   PRechecks fail
      *   change/fix code
      *   tests pass - all green
      *   convert PR to ready for review - reviewers assigned
      *   reviewers review

    Regards
    Naba



    ________________________________
    From: Owen Nichols <on...@vmware.com>
    Sent: Thursday, May 6, 2021 9:59 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode

    Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

    On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

        I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

        Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

        Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

        --Jens

        From: Owen Nichols <on...@vmware.com>
        Date: Thursday, May 6, 2021 at 9:12 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode
        A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

        Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

        I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

        Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

        On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

            Comments inline…

            Please keep your PR in draft mode anytime it is not ready to be reviewed.

            This includes if you have received request for changes, or if any PR checks are not passing.

            How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

            Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


            If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

            I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

            --Jens

Re: Reminder to use draft mode

[Mark Hanson <ha...@vmware.com>]

I have a thought. What if draft mode was the default state for the PR button and you had to select normal mode for the PR button?

Anyway, just my take.

Thanks,
Mark

On 5/6/21, 10:45 AM, "Mark Hanson" <ha...@vmware.com> wrote:

    I agree, I like the draft mode switch. The hesitations that I have are mentioned by Jens in that you can have failures that are unrelated. Especially DUnits at this point. Perhaps for required tests following the draft mode approach is better. I have had many cases where I see PRs that obviously need work asking for my review and I have to look past them in my review requested PR list. There have been several PRs that have been untouched for months that were failing tests but my review was required...

    +1 for draft mode as the best approach to get to passing tests, then normal mode once the PR is ready. (not that we are voting)

    Thanks,
    Mark

    On 5/6/21, 10:22 AM, "Nabarun Nag" <nn...@vmware.com> wrote:

        I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

        There has been this situation where,

          *   PR is created (reviewers are assigned)
          *   approved
          *   Tests fail
          *   code is changed
          *   no reviews
          *   code is merged

        Hence code that is not reviewed has been merged

        This way of doing work also has the following advantages:

          *   A reviewer does not have to review a code that causes tests to fail
          *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
          *   Unreviewed code post-test fixes do not get merged

        I think this way of working saves a critical amount of time for engineers who review code.

        This flow of PRs feels more efficient:


          *   Create PR in draft mode - no reviewers assigned
          *   PRechecks fail
          *   change/fix code
          *   tests pass - all green
          *   convert PR to ready for review - reviewers assigned
          *   reviewers review

        Regards
        Naba



        ________________________________
        From: Owen Nichols <on...@vmware.com>
        Sent: Thursday, May 6, 2021 9:59 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode

        Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

        On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

            I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

            I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

            Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

            Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

            --Jens

            From: Owen Nichols <on...@vmware.com>
            Date: Thursday, May 6, 2021 at 9:12 AM
            To: dev@geode.apache.org <de...@geode.apache.org>
            Subject: Re: Reminder to use draft mode
            A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

            Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

            I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

            Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

            On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

                Comments inline…

                Please keep your PR in draft mode anytime it is not ready to be reviewed.

                This includes if you have received request for changes, or if any PR checks are not passing.

                How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

                Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


                If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

                I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

                --Jens

Re: Reminder to use draft mode

[Mark Hanson <ha...@vmware.com>]

I agree, I like the draft mode switch. The hesitations that I have are mentioned by Jens in that you can have failures that are unrelated. Especially DUnits at this point. Perhaps for required tests following the draft mode approach is better. I have had many cases where I see PRs that obviously need work asking for my review and I have to look past them in my review requested PR list. There have been several PRs that have been untouched for months that were failing tests but my review was required...

+1 for draft mode as the best approach to get to passing tests, then normal mode once the PR is ready. (not that we are voting)

Thanks,
Mark

On 5/6/21, 10:22 AM, "Nabarun Nag" <nn...@vmware.com> wrote:

    I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

    There has been this situation where,

      *   PR is created (reviewers are assigned)
      *   approved
      *   Tests fail
      *   code is changed
      *   no reviews
      *   code is merged

    Hence code that is not reviewed has been merged

    This way of doing work also has the following advantages:

      *   A reviewer does not have to review a code that causes tests to fail
      *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
      *   Unreviewed code post-test fixes do not get merged

    I think this way of working saves a critical amount of time for engineers who review code.

    This flow of PRs feels more efficient:


      *   Create PR in draft mode - no reviewers assigned
      *   PRechecks fail
      *   change/fix code
      *   tests pass - all green
      *   convert PR to ready for review - reviewers assigned
      *   reviewers review

    Regards
    Naba



    ________________________________
    From: Owen Nichols <on...@vmware.com>
    Sent: Thursday, May 6, 2021 9:59 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode

    Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

    On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

        I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

        Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

        Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

        --Jens

        From: Owen Nichols <on...@vmware.com>
        Date: Thursday, May 6, 2021 at 9:12 AM
        To: dev@geode.apache.org <de...@geode.apache.org>
        Subject: Re: Reminder to use draft mode
        A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

        Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

        I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

        Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

        On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

            Comments inline…

            Please keep your PR in draft mode anytime it is not ready to be reviewed.

            This includes if you have received request for changes, or if any PR checks are not passing.

            How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

            Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


            If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

            I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

            --Jens

Re: Reminder to use draft mode

[Nabarun Nag <nn...@vmware.com>]

I feel that Owen has a valid point and I myself feel that it is ok to start the PR in draft mode till the pre-check tests pass.

There has been this situation where,

  *   PR is created (reviewers are assigned)
  *   approved
  *   Tests fail
  *   code is changed
  *   no reviews
  *   code is merged

Hence code that is not reviewed has been merged

This way of doing work also has the following advantages:

  *   A reviewer does not have to review a code that causes tests to fail
  *   A reviewer does not have to review code twice before failure and then again after changing the code to fix the failure
  *   Unreviewed code post-test fixes do not get merged

I think this way of working saves a critical amount of time for engineers who review code.

This flow of PRs feels more efficient:


  *   Create PR in draft mode - no reviewers assigned
  *   PRechecks fail
  *   change/fix code
  *   tests pass - all green
  *   convert PR to ready for review - reviewers assigned
  *   reviewers review

Regards
Naba



________________________________
From: Owen Nichols <on...@vmware.com>
Sent: Thursday, May 6, 2021 9:59 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode

Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

    I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

    Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

    Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

    --Jens

    From: Owen Nichols <on...@vmware.com>
    Date: Thursday, May 6, 2021 at 9:12 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode
    A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

    Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

    I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

    Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

    On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        Comments inline…

        Please keep your PR in draft mode anytime it is not ready to be reviewed.

        This includes if you have received request for changes, or if any PR checks are not passing.

        How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

        Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


        If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

        I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

        --Jens

Re: Reminder to use draft mode

[Owen Nichols <on...@vmware.com>]

Given the lack of consensus, it sounds like it will not be possible to make any assumptions about a PR based on whether it is in Draft mode or not.  I will stop retriggering flaky checks or changing PRs to draft status.  My apologies for the inconvenience this has caused.

On 5/6/21, 9:47 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

    I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

    Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

    Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

    --Jens

    From: Owen Nichols <on...@vmware.com>
    Date: Thursday, May 6, 2021 at 9:12 AM
    To: dev@geode.apache.org <de...@geode.apache.org>
    Subject: Re: Reminder to use draft mode
    A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

    Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

    I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

    Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

    On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

        Comments inline…

        Please keep your PR in draft mode anytime it is not ready to be reviewed.

        This includes if you have received request for changes, or if any PR checks are not passing.

        How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

        Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


        If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

        I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

        --Jens

Re: Reminder to use draft mode

[Jens Deppe <jd...@vmware.com>]

I don’t think we can presume everyone has the same working style. For myself I’ll happily review a PR that has a failing check. I’m OK if it has some innocuous ‘housekeeping’ error or unrelated failure.

I don’t retrigger PR failures, for unrelated errors, just to ‘get to green’ – related, I don’t expect anyone to do that on my part either. It would be frustrating if I was about to merge something and someone retriggers a job. Yes I do merge if I’m 100% confident the failed check is unrelated. I don’t merge if any checks are still pending.

Perhaps this is just relevant to my current situation, but most of my PRs are module specific and so there is collaboration between my team and we typically know the state of our various PRs. I don’t feel like there is much need for any process around switching in and out of Draft mode. Much less for an ‘external’ contributor to make decisions on our behalf.

Has some situation arisen that is driving this? It feels like there is some underlying issue that isn’t being fully communicated.

--Jens

From: Owen Nichols <on...@vmware.com>
Date: Thursday, May 6, 2021 at 9:12 AM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Reminder to use draft mode
A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    Comments inline…

    Please keep your PR in draft mode anytime it is not ready to be reviewed.

    This includes if you have received request for changes, or if any PR checks are not passing.

    How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

    Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


    If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

    I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

    --Jens

Re: Reminder to use draft mode

[Owen Nichols <on...@vmware.com>]

A PR in "Draft" mode simply conveys that at least one more commit is coming before it will be "done".  Reviewers generously volunteer their time to look at your PR, and are welcome to look at it while in draft mode if they wish, but if they are quite busy, some may prefer to wait until the PR is plausibly code-complete before setting aside time to review it.

Sorry if I wasn't clear, I don't mean that flaky failures should mean a PR is not done.  You can always refer to the latest mass test report for a list of known flaky failures, but often I will see those and retrigger them for you anyway.

I expect that most PR submitters will be monitoring their own PR checks and taking it back to draft mode as soon as they realize more changes are needed.  But if as a community we agree to use draft mode to communicate status in this way, it shouldn't matter who does it.

Due to CODEOWNERS, some reviewers have a huge number of PRs in their queue.  Clearly communicating the status of your PR allows reviewers to focus their time on PRs that are ready for review.

On 5/6/21, 8:51 AM, "Jens Deppe" <jd...@vmware.com> wrote:

    Comments inline…

    Please keep your PR in draft mode anytime it is not ready to be reviewed.

    This includes if you have received request for changes, or if any PR checks are not passing.

    How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

    Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


    If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

    I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

    --Jens

Re: Reminder to use draft mode

[Jens Deppe <jd...@vmware.com>]

Comments inline…

Please keep your PR in draft mode anytime it is not ready to be reviewed.

This includes if you have received request for changes, or if any PR checks are not passing.

How do I know if everyone is done reviewing? Or even who might be reviewing? Different reviewers may be looking at different areas, depending on the scope of the change. If the PR suddenly switches back to ‘Draft` what does that mean if I’m reviewing it? Worse still, if I’m the owner and someone else switches it to Draft I’m not notified.

Additionally, many PR checks fail for reasons unrelated to the PR so switching blindly to ‘Draft’ seems pointless.


If you’re reviewing someone’s PR, and notice any checks not passing or you are requesting changes, please also click “Convert to draft”.

I really don’t agree with this – if you have an issue with a PR for whatever reason, please respect the author and address it directly with them. I certainly feel uncomfortable ‘messing’ with someone else’s PR and, by the same token, don’t want my PRs adjusted without my input.

--Jens