How should we handle module PRs that reach "shipit" and are then rejected?

Archives Ansible Developer
1

In our current process, PRs submitted by Maintainers go directly to
"shipit" state, because we assume that Maintainers know their own
modules best, and we give them broad leeway to maintain those modules
as they see fit.

In a few cases, we have seen Committers reject these PRs once they
have reached "shipit" status. A good example is this one:

https://github.com/ansible/ansible-modules-extras/pull/1728

Submitter jtyr pushes a PR to an extras module. Because jtyr is also
the Maintainer, our triage bot puts it into "shipit". Committer bcoca
removes it from "shipit" state and puts it into "core review".

The problem here is that, because owner_pr implies "shipit", the bot
will naively place the PR back into "shipit" state continually.

So the question:

If a Committer overrules a Maintainer shipit, what should happen to
the PR? What should the process be?

Do we need some kind of new "core_block" state that explicitly says
"core has refused this PR" -- and if we have such a state, how does
the PR get out of this state?

In my opinion, core_review is currently insufficient, because we have
many many PRs placed into core_review by default, and they tend to
languish because Committers don't have time to review them.

Note that we will have many new Committers joining soon, both because
we're hiring and because we're promoting more community members to
Committer. I hope that having more Committers who can do final
"shipit" reviews will help -- but I'm seeing more and more of these
anti-shipits happening, and I want to make sure we understand the path
through.

--g

2

I believe that my code is best when reviewed by a few people. So for the modules I maintain, adding a PR that gets a shipit by default is scary. I would be more in favor of still getting a looks_good from another person before shipit, even for maintainers.

3

I believe that my code is best when reviewed by a few people. So for the
modules I maintain, adding a PR that gets a shipit by default is scary. I
would be more in favor of still getting a looks_good from another person
before shipit, even for maintainers.

Oh, yeah. This is a good point.

Now that I think about it, doing away with the auto-shipit and
requiring an explicit shipit would solve all problems, while giving
Maintainers no less control; they just have to take the extra step to
say "shipit" themselves, and that also gives them the option to leave
it open for additional comment if they so choose. I think we'll do
that! Thanks Jon :slight_smile:

--g

4

+1 for peer review, which is what I have een advocating for before.

5

Hmm

In my opinion it would make sense to give the maintainer the choice if
he wants the module to be labeled as 'shipit' or 'core_review'.

So if we do not see any 'core_review' in the comments by the maintainer,
we label it 'shipit'.

Since the core committers are usually the ones which take care of the
stability and integrity the most, they should have more voice for or
against a PR.

So remove a 'shipit' is fine, but I feel the 'core_review' is the wrong
label, it should be 'needs_revision' or 'needs_info'.

After the revision, the maintainer should be able to comment like
'ready_for_review' to get feedback from core committers (so bot would
label it as 'core_review') or comment it 'shipit' again to get the
according label.

In any way, we should try to to merge a PR by a maintainer as fast as
possible.

René

6

Hmm

In my opinion it would make sense to give the maintainer the choice if
he wants the module to be labeled as 'shipit' or 'core_review'.

So if we do not see any 'core_review' in the comments by the maintainer,
we label it 'shipit'.

Since the core committers are usually the ones which take care of the
stability and integrity the most, they should have more voice for or
against a PR.

So remove a 'shipit' is fine, but I feel the 'core_review' is the wrong
label, it should be 'needs_revision' or 'needs_info'.

Yes, I agree with this. There is an implicit "core_review" as part of
"shipit", so if the Committer has an issue with the PR, I believe that
it should be in needs_revision or needs_info.

After the revision, the maintainer should be able to comment like
'ready_for_review' to get feedback from core committers (so bot would
label it as 'core_review') or comment it 'shipit' again to get the
according label.

In any way, we should try to to merge a PR by a maintainer as fast as
possible.

My concern with the core_review label is that there are *so* many PRs
with that label.

Because "shipit" carries an implicit core review, I think I favor the
following workflow for owner PRs:

1. Maintainer submits a PR.

2. Bot responds with new boilerplate: "looks like you own this PR, but
we'll put into 'community review' anyway. If you'd like to ship this
PR without further review, you may comment with 'shipit' at any time.

3. PR goes to community_ review. Note: there's no guarantee that
anyone else will review; it's up to the Maintainer to ask for reviews,
and decide when it's ready.

4. Maintainer labels 'shipit' at some point -- maybe immediately,
maybe after review by others.

5. Committer either (a) merges, or (b) if they decide that the PR is
lacking in some way, puts it back into needs_revision.

6. Maintainer can then fix and put it either into ready_for_review and
back to 3, or shipit, and back to 4.

Does this seem reasonable?

--g

7

Sounds like a plan to me. +1

René