Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support for sending commands to bot instances via PR comments #172
support for sending commands to bot instances via PR comments #172
Changes from 55 commits
759f75d
0c7df9e
f04e622
6630509
afa6560
17d22a4
64c3d2f
c949767
c8dee13
39c7d42
f73eaa3
2702bf9
0a2c678
ff19606
e2a5782
3d9cab1
716b91f
984dbd1
931954f
e72e6a1
b44e298
7d13d7e
5988b60
3ec8b95
267a687
ddf933f
d7aeecb
79e5b57
5517e33
95beb14
7cf7f07
4801e10
b2296e4
4199d03
6e4a5d8
e1bce70
f44c311
a3e7cd3
4d84482
a744423
b042c7d
a47097f
3ab752c
0d076f7
927924c
85198eb
ac359ac
ed9f4fd
dc56784
4d40b85
d082128
975a8bc
e91ae06
a794cfd
a3c305e
236e326
d2c62e6
f8f19e4
05c6243
36851d1
13b7a27
de62337
e6c527b
1b42d77
f189f6b
684820c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are also defined in
task/build.py
, which suggests there should be a commonconstants
module to import these from?Can be done in a follow-up PR of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is triggered for every issue comment being made, it's done too early, since for accounts that don't have permission to send commands to the bot this will immediately trigger a
account XXX has NO permission to send commands to the bot
(see boegel/software-layer#24 (comment)).That should only be done if the comment effectively includes bot commands.
Let's try and make this the first thing we fix after merging this PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #184 to follow up on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of
handle_issue_comment_event
is getting quite long.How about introducing two methods there are called in
handle_issue_comment_event
. Something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, indeed.
However, the data handed over from extracting commands to processing them is not just a list of
commands
. These functions would at least needevent_info
, the comment GitHub instance (or we need to query GitHub a second time), plus possibly other state. If we move them to a separate module, we also need to take of config (now accessible asself.cfg
), logging is affected ...I agree it's long. Tend to leave it as is and rather open an issue for refactoring this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, let's open the issue.
I don't think we should move it to a separate module, it can be methods which can access
self.cfg
, but if passing around too much data is required, then maybe it doesn't make much sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should also result in a comment back to the issue/PR where the bot command was issued?
Unknown bot command 'foo', only valid bot commands are: ...
This can be done in a follow-up PR imho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, will create an issue for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should eventually be replaced by a pointer to documentation (once it's there) - can be done in follow-up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to move this stuff to
handle_bot_command
, and pass downpr
intohandle_bot_command_*
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to
event_info
andbot_command
? Maybe. Every method needs something slightly different._help
doesn't need any additional data,_build
needs theevent_info
(for passing it down to existing functions) andbot_command
._show_config
needsevent_info
. Both_build
and_show_config
useevent_info
to init an object for the pull request and pass that down to subsequent function calls. An improvement could be to put duplicate code into a functionget_pr_from_issue_event(event_info)
that returns the pull request object (we have to be a bit careful because different events have different data ... or at least name attributes differently).