Skip to content
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

Merged
merged 66 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
759f75d
initial version of action filters plus some unit tests
truib Mar 28, 2023
0c7df9e
parse updates to PR comments for bot commands
truib Mar 28, 2023
f04e622
fixing errors
truib Mar 28, 2023
6630509
add logging
truib Mar 28, 2023
afa6560
improve logging
truib Mar 28, 2023
17d22a4
more logging
truib Mar 28, 2023
64c3d2f
change determining new part in comment
truib Mar 28, 2023
c949767
allow for comment cleanup (old longer than new)
truib Mar 28, 2023
c8dee13
polish notification of command reception
truib Mar 28, 2023
39c7d42
polish more
truib Mar 28, 2023
f73eaa3
log info if line does not contain a bot command
truib Mar 29, 2023
2702bf9
also update PR comment if line does not contain a command
truib Mar 29, 2023
0a2c678
new module providing functions for managing permissions
truib Mar 29, 2023
ff19606
add a safeguard to not let the bot enter endless loops in processing …
truib Mar 29, 2023
e2a5782
make sure the bot does not act on its own comments (created/updated)
truib Mar 29, 2023
3d9cab1
change logic in testing for command permissions
truib Mar 29, 2023
716b91f
drop updating if no bot command found
truib Mar 29, 2023
984dbd1
use sender instead of author (owner) of a comment + some polishing
truib Mar 29, 2023
931954f
add missing f-string
truib Mar 29, 2023
e72e6a1
define comment_new for created action
truib Mar 29, 2023
b44e298
add hint for where to add command
truib Mar 29, 2023
7d13d7e
only scan new comments for commands + list potential commands
truib Apr 1, 2023
5988b60
init bot command object from string
truib Apr 1, 2023
3ec8b95
take a string in constructor + add filter from string
truib Apr 1, 2023
267a687
improve parsing of bot command
truib Apr 1, 2023
ddf933f
add update function for pr comments using event_info
truib Apr 1, 2023
d7aeecb
fix data format and values in case of error or empty filters
truib Apr 1, 2023
79e5b57
handling help command + smaller improvements
truib Apr 1, 2023
5517e33
outlining how to apply filter
truib Apr 1, 2023
95beb14
changed event_dir to run_dir because value iss run_dir
truib Apr 4, 2023
7cf7f07
refactoring to prepare for applying filters used with bot commands
truib Apr 4, 2023
4801e10
use filter when preparing jobs + implement build command
truib Apr 4, 2023
b2296e4
add command to show config (similar to output when PR is opened)
truib Apr 4, 2023
4199d03
name of section depends on event type
truib Apr 4, 2023
6e4a5d8
fix handling of PR opened + fix data structure for filter context
truib Apr 6, 2023
e1bce70
polishing output
truib Apr 6, 2023
f44c311
Merge branch 'main' of gh-nessi:EESSI/eessi-bot-software-layer into f…
truib Apr 6, 2023
a3e7cd3
fixing hound issue
truib Apr 6, 2023
4d84482
seems f-string {var=} is only supported from Python 3.8
truib Apr 6, 2023
a744423
add empty filter to constructor in tests
truib Apr 6, 2023
b042c7d
fix syntax of serialized action filter
truib Apr 6, 2023
a47097f
catch exception in test
truib Apr 6, 2023
3ab752c
f-string syntax {var=} not supported with Python 3.6/7
truib Apr 6, 2023
0d076f7
Merge branch 'main' of gh-nessi:EESSI/eessi-bot-software-layer into f…
truib Apr 7, 2023
927924c
removed trailing whitespaces
truib Apr 7, 2023
85198eb
fixing one test due to changed function definition
truib Apr 7, 2023
ac359ac
add base attribute to MockPullRequest
truib Apr 7, 2023
ed9f4fd
adding nested data structure used to get repository name
truib Apr 7, 2023
dc56784
change attribute name for MockPullRequest
truib Apr 7, 2023
4d40b85
fix more failing tests due to changed function definitions
truib Apr 7, 2023
d082128
fix flake8 issues
truib Apr 7, 2023
975a8bc
reduce updates to PR comments, but keep adding info to log
truib Apr 21, 2023
e91ae06
also reduce logging when handling updates to PR comments
truib Apr 21, 2023
a794cfd
try to keep update to PR comment when command is 'help'
truib Apr 21, 2023
a3c305e
Merge branch 'main' of gh-nessi:EESSI/eessi-bot-software-layer into f…
truib May 12, 2023
236e326
various updates to address review of PR172
truib May 29, 2023
d2c62e6
use changed function name
truib May 30, 2023
f8f19e4
polish messages of bot + fixing minor issues
truib May 30, 2023
05c6243
improve formatting of results to bot commands
truib May 30, 2023
36851d1
adjust pytests to take changed return value into account
truib May 30, 2023
13b7a27
use changed attribute name for comment id
truib May 31, 2023
de62337
IssueComment only has the attribute id, so renaming it in tests
truib May 31, 2023
e6c527b
also report to PR on GitHub if command sender has no permission
truib May 31, 2023
1b42d77
ensure that the bot does not comment on its own comments
truib May 31, 2023
f189f6b
adding information about configuring the feature to send commands
truib May 31, 2023
684820c
added check to only run update_pr_comment for issue_comment events
truib May 31, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,27 @@ submit_command = /usr/bin/sbatch
```
This is the full path to the Slurm command used for submitting batch jobs. You may want to verify if `sbatch` is provided at that path or determine its actual location (`which sbatch`).

### Section `[bot_control]`
The section `[bot_control]` contains settings for configuring the feature to
send commands to the bot.
```
command_permission = GH_ACCOUNT_1 GH_ACCOUNT_2 ...
```
The option `command_permission` defines which GitHub accounts can send commands
to the bot (via new PR comments). If the value is empty NO account can send
commands.

```
command_response_fmt = FORMAT_MARKDOWN_AND_HTML
```
This allows to customize the format of the comments about the handling of bot
commands. The format needs to include `{sender}`, `{comment_response}` and
`{comment_result}`. `{app_name}` is replaced with the name of the bot instance.
`{comment_response}` is replaced with information about parsing the comment
for commands before any command is run. `{comment_result}` is replaced with
information about the result of the command that was run (can be empty).


### Section `[deploycfg]`
The section `[deploycfg]` defines settings for uploading built artefacts (tarballs).
```
Expand Down
20 changes: 20 additions & 0 deletions app.cfg.example
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,26 @@ installation_id = 12345678
private_key = PATH_TO_PRIVATE_KEY


[bot_control]
# which GH accounts have the permission to send commands to the bot
# if value is left/empty everyone can send commands
# value can be a space delimited list of GH accounts
# NOTE, be careful to not list the bot's name (GH app name) or it may consider
# comments created/updated by itself as a bot command and thus enter an
# endless loop.
command_permission =

# format of the response when processing bot commands. NOTE, make sure the
# placeholders {app_name}, {comment_response} and {comment_result} are included.
command_response_fmt =
<details><summary>Updates by the bot instance <code>{app_name}</code>
<em>(click for details)</em></summary>

{comment_response}
{comment_result}
</details>


[buildenv]
# name of the job script used for building an EESSI stack
build_job_script = PATH_TO_EESSI_BOT/scripts/bot-build.slurm
Expand Down
244 changes: 233 additions & 11 deletions eessi_bot_event_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,26 @@
import tasks.deploy as deploy

from connections import github
from tasks.build import check_build_permission, get_architecture_targets, get_repo_cfg, submit_build_jobs
from tasks.deploy import deploy_built_artefacts
from tools import config
from tools.args import event_handler_parse
from tasks.build import check_build_permission, submit_build_jobs, get_repo_cfg
from tasks.deploy import deploy_built_artefacts
from tools.commands import EESSIBotCommand, EESSIBotCommandError, get_bot_command
from tools.filter import EESSIBotActionFilter
from tools.permissions import check_command_permission
from tools.pr_comments import create_comment

from pyghee.lib import PyGHee, create_app, get_event_info, read_event_from_json
from pyghee.utils import log


APP_NAME = "app_name"
Copy link
Contributor

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 common constants module to import these from?

Can be done in a follow-up PR of course

BOT_CONTROL = "bot_control"
COMMAND_RESPONSE_FMT = "command_response_fmt"
GITHUB = "github"
REPO_TARGET_MAP = "repo_target_map"
boegel marked this conversation as resolved.
Show resolved Hide resolved


class EESSIBotSoftwareLayer(PyGHee):

def __init__(self, *args, **kwargs):
Expand Down Expand Up @@ -60,10 +71,151 @@ def handle_issue_comment_event(self, event_info, log_file=None):
"""
request_body = event_info['raw_request_body']
issue_url = request_body['issue']['url']
comment_author = request_body['comment']['user']['login']
comment_txt = request_body['comment']['body']
self.log("Comment posted in %s by @%s: %s", issue_url, comment_author, comment_txt)
self.log("issue_comment event handled!")
action = request_body['action']
sender = request_body['sender']['login']
owner = request_body['comment']['user']['login']
repo_name = request_body['repository']['full_name']
pr_number = request_body['issue']['number']

# FIXME add request body text (['comment']['body']) to log message when
# log level is set to debug
self.log(f"Comment in {issue_url} (owned by @{owner}) {action} by @{sender}")

# obtain app name and format for reporting about received & processed
# commands
app_name = self.cfg[GITHUB][APP_NAME]
command_response_fmt = self.cfg[BOT_CONTROL][COMMAND_RESPONSE_FMT]

# currently, only commands in new comments are supported
# - commands have the syntax 'bot: COMMAND [ARGS*]'

# first check if sender is authorized to send any command
# - this serves a double purpose:
# 1. check permission
# 2. skip any comment updates that were done by the bot itself
# --> thus we prevent the bot from entering an endless loop
# where it reacts on updates to comments it made itself
# NOTE this assumes that the sender of the event is corresponding to
# the bot if the bot updates comments itself and that the bot is not
# given permission in the configuration setting 'command_permission'
# ... in order to prevent surprises we should be careful what the bot
# adds to comments, for example, before updating a comment it could
# run the update through the function checking for a bot command.
boegel marked this conversation as resolved.
Show resolved Hide resolved
if check_command_permission(sender) is False:
Copy link
Contributor

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...

Copy link
Contributor

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

self.log(f"account `{sender}` has NO permission to send commands to bot")
# need to ensure that the bot is not responding on its own comments
# as a quick implementation we check if the sender name contains '[bot]'
# FIXME improve this by querying (and caching) information about the sender of
# an event ALTERNATIVELY we could postpone this test a bit until we
# have parsed the comment and know if it contains any bot command
if not sender.endswith('[bot]'):
comment_response = f"\n- account `{sender}` has NO permission to send commands to the bot"
comment_body = command_response_fmt.format(
app_name=app_name,
comment_response=comment_response,
comment_result=''
)
issue_comment = create_comment(repo_name, pr_number, comment_body)
else:
self.log(f"account `{sender}` seems to be a bot instance itself, hence not creating a new PR comment")
return
else:
self.log(f"account `{sender}` has permission to send commands to bot")

# only scan for commands in newly created comments
if action == 'created':
comment_received = request_body['comment']['body']
self.log(f"comment action '{action}' is handled")
else:
# NOTE we do not report this with a new PR comment, because otherwise any update to a comment would
# let the bot add a response (would be very noisy); we might add a debug mode later
self.log(f"comment action '{action}' not handled")
return

# search for commands in comment
comment_response = ''
commands = []
# process non-empty (if x) lines (split) in comment
for line in [x for x in [y.strip() for y in comment_received.split('\n')] if x]:
# FIXME add processed line(s) to log when log level is set to debug
bot_command = get_bot_command(line)
if bot_command:
try:
ebc = EESSIBotCommand(bot_command)
except EESSIBotCommandError as bce:
self.log(f"ERROR: parsing bot command '{bot_command}' failed with {bce.args}")
# FIXME possibly add more information to log when log level is set to debug
comment_response += f"\n- parsing the bot command `{bot_command}`, received"
comment_response += f" from sender `{sender}`, failed"
continue
commands.append(ebc)
self.log(f"found bot command: '{bot_command}'")
comment_response += f"\n- received bot command `{bot_command}`"
comment_response += f" from `{sender}`"
comment_response += f"\n - expanded format: `{ebc.to_string()}`"
# FIXME add an else branch that logs information for comments not
# including a bot command; the logging should only be done when log
# level is set to debug

if comment_response == '':
# no update to be added, just log and return
self.log("comment response is empty")
return
else:
boegel marked this conversation as resolved.
Show resolved Hide resolved
self.log(f"comment response: '{comment_response}'")

if not any(map(get_bot_command, comment_response.split('\n'))):
# the 'not any()' ensures that the response would not be considered a bot command itself
# ... together with checking the sender of a comment update this aims
# at preventing the bot to enter an endless loop in commenting on its own
# comments
comment_body = command_response_fmt.format(
app_name=app_name,
comment_response=comment_response,
comment_result=''
)
issue_comment = create_comment(repo_name, pr_number, comment_body)
else:
self.log(f"update '{comment_response}' is considered to contain bot command ... not creating PR comment")
# FIXME we may want to report this back to the PR on GitHub, e.g.,
# "Oops response message seems to contain a bot command. It is not
# displayed here to prevent the bot from entering an endless loop
# of commands. Please, check the logs at the bot instance for more
# information."

# process commands
comment_result = ''
for cmd in commands:
Copy link
Contributor

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:

commands = self.extract_bot_commands(...)
self.log("Found %d bot commands in comment ..." % len(commands))
self.process_bot_commands(commands)

Copy link
Contributor Author

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 need event_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 as self.cfg), logging is affected ...

I agree it's long. Tend to leave it as is and rather open an issue for refactoring this later.

Copy link
Contributor

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.

try:
update = self.handle_bot_command(event_info, cmd)
comment_result += f"\n- handling command `{cmd.to_string()}` resulted in: "
comment_result += update
self.log(f"handling command '{cmd.to_string()}' resulted in '{update}'")

except EESSIBotCommandError as err:
self.log(f"ERROR: handling command {cmd.command} failed with {err.args[0]}")
comment_result += f"\n- handling command `{cmd.command}` failed with message"
comment_result += f"\n _{err.args[0]}_"
continue
except Exception as err:
log(f"Unexpected err={err}, type(err)={type(err)}")
if comment_result:
comment_body = command_response_fmt.format(
app_name=app_name,
comment_response=comment_response,
comment_result=comment_result
)
issue_comment.edit(comment_body)
raise
# only update PR comment once
comment_body = command_response_fmt.format(
app_name=app_name,
comment_response=comment_response,
comment_result=comment_result
)
issue_comment.edit(comment_body)

self.log(f"issue_comment event (url {issue_url}) handled!")

def handle_installation_event(self, event_info, log_file=None):
"""
Expand All @@ -88,7 +240,8 @@ def handle_pull_request_labeled_event(self, event_info, pr):
if label == "bot:build":
# run function to build software stack
if check_build_permission(pr, event_info):
submit_build_jobs(pr, event_info)
# use an empty filter
submit_build_jobs(pr, event_info, EESSIBotActionFilter(""))

elif label == "bot:deploy":
# run function to deploy built artefacts
Expand All @@ -101,13 +254,20 @@ def handle_pull_request_opened_event(self, event_info, pr):
Handle opening of a pull request.
"""
self.log("PR opened: waiting for label bot:build")
app_name = self.cfg['github']['app_name']
app_name = self.cfg[GITHUB][APP_NAME]
# TODO check if PR already has a comment with arch targets and
# repositories
arch_map = get_architecture_targets(self.cfg)
repo_cfg = get_repo_cfg(self.cfg)

comment = f"Instance `{app_name}` is configured to build:"
for arch in repo_cfg['repo_target_map'].keys():
for repo_id in repo_cfg['repo_target_map'][arch]:

for arch in arch_map.keys():
# check if repo_target_map contains an entry for {arch}
if arch not in repo_cfg[REPO_TARGET_MAP]:
self.log(f"skipping arch {arch} because repo target map does not define repositories to build for")
continue
for repo_id in repo_cfg[REPO_TARGET_MAP][arch]:
comment += f"\n- arch `{'/'.join(arch.split('/')[1:])}` for repo `{repo_id}`"

self.log(f"PR opened: comment '{comment}'")
Expand All @@ -117,7 +277,8 @@ def handle_pull_request_opened_event(self, event_info, pr):
gh = github.get_instance()
repo = gh.get_repo(repo_name)
pull_request = repo.get_pull(pr.number)
pull_request.create_issue_comment(comment)
issue_comment = pull_request.create_issue_comment(comment)
return issue_comment

def handle_pull_request_event(self, event_info, log_file=None):
"""
Expand All @@ -138,6 +299,67 @@ def handle_pull_request_event(self, event_info, log_file=None):
else:
self.log("No handler for PR action '%s'", action)

def handle_bot_command(self, event_info, bot_command, log_file=None):
"""
Handle bot command

Args:
event_info (dict): object containing all information of the event
bot_command (EESSIBotCommand): command to be handled
"""
cmd = bot_command.command
handler_name = f"handle_bot_command_{cmd}"
if hasattr(self, handler_name):
handler = getattr(self, handler_name)
self.log(f"Handling bot command {cmd}")
return handler(event_info, bot_command)
else:
self.log(f"No handler for command '{cmd}'")
raise EESSIBotCommandError(f"unknown command `{cmd}`; use `bot: help` for usage information")

def handle_bot_command_help(self, event_info, bot_command):
boegel marked this conversation as resolved.
Show resolved Hide resolved
"""handles command 'bot: help' with a simple usage info"""
help_msg = "\n **How to send commands to bot instances**"
help_msg += "\n - Commands must be sent with a **new** comment (edits of existing comments are ignored)."
help_msg += "\n - A comment may contain multiple commands, one per line."
help_msg += "\n - Every command begins at the start of a line and has the syntax `bot: COMMAND [ARGUMENTS]*`"
help_msg += "\n - Currently supported COMMANDs are: `help`, `build`, `show_config`"
return help_msg

def handle_bot_command_build(self, event_info, bot_command):
boegel marked this conversation as resolved.
Show resolved Hide resolved
"""handles command 'bot: build [ARGS*]' by parsing arguments and submitting jobs"""
gh = github.get_instance()
boegel marked this conversation as resolved.
Show resolved Hide resolved
self.log("repository: '%s'", event_info['raw_request_body']['repository']['full_name'])
repo_name = event_info['raw_request_body']['repository']['full_name']
pr_number = event_info['raw_request_body']['issue']['number']
pr = gh.get_repo(repo_name).get_pull(pr_number)
build_msg = ''
if check_build_permission(pr, event_info):
# use filter from command
submitted_jobs = submit_build_jobs(pr, event_info, bot_command.action_filters)
if submitted_jobs is None or len(submitted_jobs) == 0:
build_msg = "\n - no jobs were submitted"
else:
for job_id, issue_comment in submitted_jobs.items():
build_msg += f"\n - submitted job `{job_id}`"
if issue_comment:
build_msg += f", for details & status see {issue_comment.html_url}"
else:
request_body = event_info['raw_request_body']
sender = request_body['sender']['login']
build_msg = f"\n - account `{sender}` has NO permission to submit build jobs"
return build_msg

def handle_bot_command_show_config(self, event_info, bot_command):
"""handles command 'bot: show_config' by printing a list of configured build targets"""
self.log("processing bot command 'show_config'")
gh = github.get_instance()
repo_name = event_info['raw_request_body']['repository']['full_name']
pr_number = event_info['raw_request_body']['issue']['number']
pr = gh.get_repo(repo_name).get_pull(pr_number)
Copy link
Contributor

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 down pr into handle_bot_command_*?

Copy link
Contributor Author

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 and bot_command? Maybe. Every method needs something slightly different. _help doesn't need any additional data, _build needs the event_info (for passing it down to existing functions) and bot_command. _show_config needs event_info. Both _build and _show_config use event_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 function get_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).

issue_comment = self.handle_pull_request_opened_event(event_info, pr)
return f"\n - added comment {issue_comment.html_url} to show configuration"

def start(self, app, port=3000):
"""starts the app and log information in the log file

Expand Down
Loading