Found while running triage on core. Using the new and improved prbot.py freshly updated yesterday.

3099 --- Fix: Incorrect parameters for module.set_fs_attributes_if_different in file.py
Labels: []
Submitter: bassco
Maintainer(s): ansible
Filename(s): files/file.py

RECOMMENDED ACTIONS for ansible/ansible-modules-core#3099
newlabel: core_review
boilerplate: core_review_existing

Take recommended actions (y/N)?y
LABELS_URL: https://api.github.com/repos/ansible/ansible-modules-core/issues/3099/labels{/name}
COMMENTS_URL: https://api.github.com/repos/ansible/ansible-modules-core/issues/3099/comments
Traceback (most recent call last):
File "./prbot.py", line 587, in
triage(shortpull['url'])
File "./prbot.py", line 531, in triage
mtext = pr_maintainers.replace(' ', ' @')
AttributeError: 'list' object has no attribute 'replace'

Some instances of a PR being put into a review state when it's already in a needs state:

• (e1793, e1722, c2438, c2278, c1665) Trying to take a PR in "needs_info" state and putting it back into community_review/core_review. Bot should leave it in "needs_info" until we get an explicit ready_for_review.
• (e1663, c3015) Trying to take a PR in "needs_revision" state and putting it back into community_review. Not sure why.
• (e1480) Trying to take a PR in "needs_rebase" state and putting it back into community_review. Not sure why.

Sometimes a PR just isn't going to be accepted; maybe it's a duplicate, for instance. In that case, we should provide a mechanism for the maintainer of the module to request closure of the module.

I've hit the limit 2-3x in the past week and had to basically stop midstream (and then go back from the beginning, ugh).

Not a fire but sure would be nice. :)

If CI has failed on a PR, we should indicate that to the user, and not proceed with adding the shipit label.

Pretty sure this is not happening automagically just yet.

I've already seen three issues today that were filed in core instead of extras.

If it's not mergeable, it's not ready to ship. :)

Whole bunch of new network modules need to have maintainers added to the list. Including:

cumulus
ios
iosxr
junos
nxos
openswitch

ansible/ansible-modules-extras#1707 is an example.

Text currently says:

"Thanks @submitterofstuffandthings for this PR. This module is maintained by the Ansible core team, so it can take a while for patches to be reviewed. Thanks for your patience.

[This message brought to you by your friendly Ansibull-bot.]"

It should probably say,

"Thanks @submitterofstuffandthings for this PR. This module is maintained by the Ansible core team, so it can take a while for patches to be reviewed. Thanks for your patience.

Core team: please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]"

And this is for a few reasons:

• Keep consistency for community_review and core_review items; we don't want to confuse issue submitters with what may appear to be different types of workflow.
• Core folks should be using the proper bot word tags to ensure that the bot workflow behaves correctly and providing text to also appropriately prompt submitters / issue creators to do the right thing. Additionally, even if the core team "knows" that they should do this, it still lacks consistency (as mentioned above) and the addition of more core team members may mean that a new member may not know to do this.

Also:

• Should also ask core team to not manually change labels if possible to avoid breaking bot workflow.
• Not sure how this applies in the shipit case.

The "friendly reminder" prompt should remind people that when they do eventually address the issues with the PR, they should leave a comment stating "ready_for_review" so that it can be handled properly.

ansible/ansible-modules-extras#1354 is an example of how the lack of this can go awry. In this particular case, the maintainer changed from previous maintainer to the core team (also, when we reassign it to the core team, we should prompt them to tell the submitter to say ready_for_review as well, but I'll make a separate issue for that.), who then commented on some diffs, which then got updated, but the submitter never stated ready_for_review, and thus the maintainer has not been pinged for review.

Text currently says:
"@submitter: A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes.

[This message brought to you by your friendly Ansibull-bot.]"

It should say:
"@submitter: A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer."

[This message brought to you by your friendly Ansibull-bot.]"

ansible/ansible-modules-core#2586 <- as seen here, not the first time I've seen such requirements.

How do we handle that with ansibullbot? special snowflake label?

2:35 PM gregdek: ansibull-bot marked 'shipit' on a PR that was only meant to be a comment
2:35 PM ansible/ansible-modules-extras#1370 (comment)
2:35 PM it was just that good
2:35 PM → cloudtrainme joined ⇐ kushal quit
2:36 PM hehe. ansibull-bot is 2 days early. april 1st isn't until friday
2:36 PM → josuebrunel joined ⇐ ageorgop quit
2:36 PM <•gregdek> Ah, thanks. Looks like the bot is too aggressively giving "shipits" for new modules. resmo: ^^^
2:36 PM → ppinkert_ joined (~[email protected])
2:37 PM <•gregdek> Gave "owner_pr shipit" to a module that isn't a module yet. :)

For modules that are maintained by core, we really need some kind of "adopt_me" boilerplate.

Then, if a non-bot user says "adopt_me", we label it somehow and kick it into a separate module adoption process (details tba).

...and do the right thing.

Since some maintainers can't seem to follow instructions :)

I did a deeper review how the prbot is working and I found that it does not handle state correctly.

The PR defines actions based on current comments and labels, it also handles boilerplates e.g. warning for current comments an label.

However it does not implicate the actions.

As a result of it, the bot could suggest giving a warning for a PR in community_review even if the actions has a shipit. I also found that if the maintainer of a module changes, it is not going to be handled correctly.

(Ironically this is what all ansible modules have to handle and what ansible is all about... :) )

So how we can fix that:

1. we find the current state
2. find out in which state the PR should be aka desired state
3. we get the path from current state to desired state by defining actions.

I don't see a easy way to implement this properly in the current bot. I plan to give it a rewrite this weekend.

However I also make a PR for a few problems I found while testing the current bot.

ansible/ansible-modules-core#2548

We should be getting automated metrics from this as well. We should collect:

• Number of issues in each state
• Average open time?

...and put into a spreadsheet somewhere.

This is probably best for when it's fully automated.

Right now we break for all PRs for which there are no actions recommended.

We could save a lot of time if we just skip over those PRs with a summary at the end of the PRs that we skipped, and streamline the output.

Seems as though we're not handling needs_rebase correctly:

(This example may be outdated quickly)

https://api.github.com/repos/ansible/ansible-modules-core/pulls/2230
...shows that pull['mergeable'] should be 'false'.

But we don't detect it. Why not?

...would be useful and nice.

There are many cases in which the maintainer changes while a PR is midflight. The bot should catch this case and handle it properly.

If someone from the core team comments on a PR that would otherwise be in community_review -- should core then own that PR? Need to make a policy decision here.

See ansible/ansible-modules-extras#1160. Should prbot be checking for this?

Any request that comes in for a backport to Ansible 1.9 (or perhaps any non-devel branch) should be handed directly to the core team with its own boilerplate, and should also be labeled "backport".

No useful state change since last pass can sometimes be due to the person previously maintaining it is no longer the maintainer, but the new maintainer has not been pinged. (See ansible/ansible-modules-core#2639.)

Alternative solutions:

• Verify every time the bot runs through a PR that the current maintainer has been pinged / mentioned at least once by the bot in a comment, so that when maintainership transfers/additions happen, the new person can also be pinged.
• When a line in the maintainers file changes, have a bot search all the PRs for old / previous maintainers, ping new / updated list of maintainers.

We don't want people trying to review something that's in needs_rebase.

We can always have exception cases, but the tool should by default say "no thanks, please submit to extras" when it sees a new module request in core.

Re: ansible/ansible-modules-core#2748, ansible/ansible-modules-core#2747 - bot is not handling this situation properly, wants to assign it to someone when the response should simply be that the module is deprecated.

Note: may be a bunch of corner cases on this to think about, but. :)

Default behavior when we don't find a match in the MAINTAINERS file is to assume that we're dealing with a new module. This is incorrect.

The right thing to do: look at the file diff. If there's no code in the a: side of the diff, then and only then should we assume new module. If we find a file that's clearly existing with no maintainer, we should warn appropriately.

The goal of the "WIP" (work in progress) state is to tell the bot to just leave it alone for now. Proposal:

• Any PR with "WIP or [WIP]" at the start of the title is labeled "WIP"
• Any PR labeled "WIP" but does not have "WIP" in the title has label removed
• Boilerplate telling people "this PR is a work-in-progress. @submitter, when you're ready to have this PR reviewed, please remove WIP from the title of the PR."
• Any PR in WIP is otherwise ignored completely by the bot.

Right now, we have no way of looking at travis failures.

The truncated boilerplate is perhaps too short -- need to adjust it still.

Sometimes you need to debug the triaging of a single PR, and you don't want to go over the entire list just to get to the wonky PR. Add a mode in which you can specify and handle a single PR (or maybe a list of PRs.)

Title says it all. We don't encourage new modules in Core, so they should go immediately into core_review (where the core team can say "yes" or "please refile in extras").

He has a lot of opens, and he's not re

Here's the workflow, I think:

• Issue touched by no one in two weeks?
  • If community_review and new_plugin, ignore (we need something better here)
  • If community_review and not new_plugin and not pending_action:
    • If <=2 last comments are bot pings, ping maintainer
    • If >2 last comments are bot pings, put in pending_action
  • If community_review and not new_plugin and pending_action:
    • Flag "orphaned" (and figure this process out!)
  • If core_review, touch ("still waiting for core team, thx for your patience") and move on.
  • If needs_* and not pending_action, put in pending_action
  • If needs_* and pending_action, close

Bot keeps trying to remove pending_action labels. Example: Core 1985.

When a bot user puts something into pending action, we should leave it there until it's removed by a subsequent submitter action, a reviewer action, or a bot action.

Take a look

Per discussion on mailing list:

• do away with owner_pr tag, as it's superfluous
• new owner_pr boilerplate that says: "This module is going into community review. We notice that you are maintainer of this module, so when you think it's ready, please comment "shipit" and we will consider it for merging."
• change the current "owner_pr" behavior to put an owner_pr into community_review instead of shipit.

Probably the right thing to do, for now, is just to flag them every single time. "Hey, this is still in needs_info, should we do something about it?" The goal is always to resolve needs_info into some other state as quickly as possible -- and anything that stays in needs_info for too long should probably just be closed by hand.

See ansible/ansible-modules-core#2653 -

Note that:

• Mark filed a PR to remove himself as maintainer and added ansible core team to some mysql modules
• Brian merged it
• maintainers_core, on the other hand, lists jmainguy as the maintainer

(This, ultimately, has hosed PRs such as ansible/ansible-modules-core#2639 -- who should be pinging someone new, presumably jmainguy?)

Alternately: We remove the maintainer line out of all modules?

Sometimes something will be pushed into community_review for two different reasons -- typical case appears to be that needs_rebase is fixed, and the submitter says "ready_for_review". Should set the action queue to dedup before taking actions.

Need to figure out how to deal gracefully with multi-file PRs at some point.

In the cases of modules or files with multiple maintainers, when one of the co-maintainers submits a PR (and thus, gets labeled owner_pr and is merged automagically), when we apply the shipit label we should ping the other co-maintainers as a polite heads-up.

(I don't know if this means we should handle the case where one of those maintainers then wants to move it out of shipit in the short time period between shipit label and actual merge, or if it can still be moved out of shipit via a needs_revision comment at that point or not, at least as the code stands today.)

ansible/ansible-modules-extras#1354 is an example.

The maintainer was changed to the core team, who requested revisions in a diff, but did not state the regular boilerplate that is normally given to maintainers ("please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.") In this case the core team manually changed the label to needs_revision, but didn't state needs_revision, and thus the maintainer was not pinged for some time until the bot noticed that it was in a needs_revision state for X number of days, and then it prompted him.

Perhaps this should be an "anytime the reviewer is changed from community_review label to core_review label, add core_review boilerplate text" (which should be fixed also, because it says nothing about shipit vs. needs_revision, but that's yet another ticket.)

With #35 -- prbot now has additional options that should be specified at runtime.

(Yes, they actually print out when you run the thing, but still. :D)

usage: prbot.py [-h] [--ghuser GHUSER] [--ghpass GHPASS] [--ghtoken GHTOKEN]
[--ghrepo {core,extras}] [--verbose] [--debug] [--pause]
[--pr PR] [--startat STARTAT]

New bot seems to ignore the case when an existing module does not yet have a maintainer listed.

Previous bot treated this as FATAL, and I would immediately stop the run and edit the appropriate maintainers file.

New behavior should probably be to skip and and send some kind of notification to the bot team.