# How should we handle module PRs that reach "shipit" and are then rejected? **URL:** \ **Category:** Ansible Developer\ **Created:** [March 14, 2016, 6:07pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864 "2016-03-14T18:07:08Z")\ **Posts on this page:** 7\ **Page:** 1
**Author:** ![Greg\_DeKoenigsberg1](https://avatars.discourse-cdn.com/v4/letter/g/838e76/32.png) [@Greg\_DeKoenigsberg1](https://forum.ansible.com/u/Greg_DeKoenigsberg1)\ **Post date:** [March 14, 2016, 6:07pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864/1 "2016-03-14T18:07:08Z")
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](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 ---
**Author:** ![Jonathan\_Mainguy](https://avatars.discourse-cdn.com/v4/letter/j/e274bd/32.png) [@Jonathan\_Mainguy](https://forum.ansible.com/u/Jonathan_Mainguy)\ **Post date:** [March 14, 2016, 8:04pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864/2 "2016-03-14T20:04:51Z")
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. ---
**Author:** ![Greg\_DeKoenigsberg1](https://avatars.discourse-cdn.com/v4/letter/g/838e76/32.png) [@Greg\_DeKoenigsberg1](https://forum.ansible.com/u/Greg_DeKoenigsberg1)\ **Post date:** [March 14, 2016, 9:05pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864/3 "2016-03-14T21:05:12Z")
> 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 đŸ™‚ --g ---
**Author:** ![Serge\_van\_Ginderacht](https://avatars.discourse-cdn.com/v4/letter/s/b782af/32.png) [@Serge\_van\_Ginderacht](https://forum.ansible.com/u/Serge_van_Ginderacht)\ **Post date:** [March 14, 2016, 9:08pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864/4 "2016-03-14T21:08:39Z")
+1 for peer review, which is what I have een advocating for before. ---
**Author:** ![Rene\_Moser](https://avatars.discourse-cdn.com/v4/letter/r/7bcc69/32.png) [@Rene\_Moser](https://forum.ansible.com/u/Rene_Moser)\ **Post date:** [March 14, 2016, 9:22pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864/5 "2016-03-14T21:22:59Z")
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é ---
**Author:** ![Greg\_DeKoenigsberg1](https://avatars.discourse-cdn.com/v4/letter/g/838e76/32.png) [@Greg\_DeKoenigsberg1](https://forum.ansible.com/u/Greg_DeKoenigsberg1)\ **Post date:** [March 15, 2016, 2:43pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864/6 "2016-03-15T14:43:45Z")
> 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 ---
**Author:** ![Rene\_Moser](https://avatars.discourse-cdn.com/v4/letter/r/7bcc69/32.png) [@Rene\_Moser](https://forum.ansible.com/u/Rene_Moser)\ **Post date:** [March 15, 2016, 5:15pm UTC](https://forum.ansible.com/t/how-should-we-handle-module-prs-that-reach-shipit-and-are-then-rejected/20864/7 "2016-03-15T17:15:56Z")
Sounds like a plan to me. +1 René