-
Notifications
You must be signed in to change notification settings - Fork 22
Add new bot: cancel command
#359
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
Conversation
trz42
left a comment
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.
Congrats @sondrebr, this looks like a solid PR.
There are a few minor suggestions to consider for this PR. I noted a few other things, which we can address in follow-up PRs.
Co-authored-by: Thomas Röblitz <trz42@users.noreply.github.com>
Co-authored-by: Thomas Röblitz <trz42@users.noreply.github.com>
- Renamed `FILTER_COMPONENT_JOB` to `FILTER_COMPONENT_JOBID`
casparvl
left a comment
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.
I haven't done a review of the code, but I did test it here casparvl/software-layer#6 (comment) (cancelled this job here casparvl/software-layer#6 (comment)) and it works perfectly fine! One thing I wasn't able to test is if someone that has permissions to command the bot but was not the submitter of the job can indeed not cancel the job (which was your design goal). But since you need two people for that, maybe it's easier if you test that in some face-to-face session with @trz42 :)
Anyway, nice feature! :)
trz42
left a comment
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.
|
Testing actually revealed a little issue. It tells something about See trz42/software-layer#93 (comment) Can you have a look @sondrebr ? |
I think I found a "fix" for this by simply updating a configuration setting. The change below diff --git a/app.cfg.example b/app.cfg.example
index a5b1441..f4981d6 100644
--- a/app.cfg.example
+++ b/app.cfg.example
@@ -164,7 +164,7 @@ cancel_command = /usr/bin/scancel
build_permission = -NOT_ALLOWED_GH_ACCOUNT_NAME-
# template for comment when user who set a label has no permission to trigger build jobs
-no_build_permission_comment = Label `bot:build` has been set by user `{build_labeler}`, but this person does not have permission to trigger builds
+no_build_permission_comment = GH account `{build_labeler}` is not authorized to trigger or cancel build jobs.
# whether or not to allow updating the submit options via custom module det_submit_opts
# Should only be enabled (true) with care because this will result in code from the targetleads to a different message as shown in trz42/software-layer#93 (comment) So my suggestion is to update the setting in We also need to make users aware of this (that they should update the setting or could receive misleading reply) in the notes for the next release (to be created after this PR gets merged). |
|
Good catch! I have updated the relevant bot comments 👍 |
trz42
left a comment
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.
Nice work!
See issue #190
The command allows for cancelling multiple jobs in one command, e.g.
bot: cancel jobid:1 jobid:2. The status table of each job is updated to show that the job was cancelled. A build job can only be cancelled by the job owner (the one who submitted it). This is done by adding the name of the job owner to the job metadata file, and checking that the name matches before cancelling the job.A new setting is required for the command, 'cancel_command' in the '[buildenv]' section.
One small issue at the moment is that jobs that are known by the job manager get
UNKNOWNstatus when they are cancelled, but I guess that can be easily ignored and fixed in another PR if needed.I have (hopefully) updated the documentation and tests where necessary, although I haven't added any new tests.
First PR, sorry for the single huge commit 😅 Any feedback would be appreciated 🙂