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 <hans...@vmware.com>
Date: Thursday, May 6, 2021 at 10:45 AM
To: dev@geode.apache.org <dev@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" <n...@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 <onich...@vmware.com>
Sent: Thursday, May 6, 2021 9:59 AM
To: dev@geode.apache.org <dev@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" <jde...@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 <onich...@vmware.com>
Date: Thursday, May 6, 2021 at 9:12 AM
To: dev@geode.apache.org <dev@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" <jde...@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
