ansible / ansibullbot Goto Github PK
View Code? Open in Web Editor NEWBot for management of Ansible issues and PRs on GitHub.
License: GNU General Public License v3.0
Bot for management of Ansible issues and PRs on GitHub.
License: GNU General Public License v3.0
See ansible/ansible-modules-extras#1160. Should prbot be checking for this?
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. :)
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.)
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.]"
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.)
Some instances of a PR being put into a review state when it's already in a needs state:
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.
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'
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-core#2586 <- as seen here, not the first time I've seen such requirements.
How do we handle that with ansibullbot? special snowflake label?
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?
If CI has failed on a PR, we should indicate that to the user, and not proceed with adding the shipit
label.
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.
The goal of the "WIP" (work in progress) state is to tell the bot to just leave it alone for now. Proposal:
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).
Per discussion on mailing list:
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.
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.
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:
Also:
The truncated boilerplate is perhaps too short -- need to adjust it still.
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]
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:
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.
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").
Here's the workflow, I think:
We should be getting automated metrics from this as well. We should collect:
...and put into a spreadsheet somewhere.
This is probably best for when it's fully automated.
Pretty sure this is not happening automagically just yet.
...and do the right thing.
Take a look
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.
Since some maintainers can't seem to follow instructions :)
Whole bunch of new network modules need to have maintainers added to the list. Including:
cumulus
ios
iosxr
junos
nxos
openswitch
...would be useful and nice.
If it's not mergeable, it's not ready to ship. :)
Right now, we have no way of looking at travis failures.
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.
We don't want people trying to review something that's in needs_rebase.
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.
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".
Need to figure out how to deal gracefully with multi-file PRs at some point.
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:
See ansible/ansible-modules-core#2653 -
Note that:
(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?
There are many cases in which the maintainer changes while a PR is midflight. The bot should catch this case and handle it properly.
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.
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.
He has a lot of opens, and he's not re
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. :)
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. :)
I've already seen three issues today that were filed in core instead of extras.
A declarative, efficient, and flexible JavaScript library for building user interfaces.
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google ❤️ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.