-
Notifications
You must be signed in to change notification settings - Fork 875
feat(cli): add provisioner job cancel command #16252
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
20596bf
to
d50b500
Compare
d50b500
to
efdcbba
Compare
coderd/provisionerjobs.go
Outdated
return | ||
} | ||
|
||
jobs, ok := api.getProvisionerJobs(rw, r, []uuid.UUID{jobID}) |
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.
Review: Note that I'm sharing logic between this endpoint and the one for returning multiple. I mainly did this to ensure they use the same authorization code between the endpoints. That's because the existing api.Database.GetProvisionerJobByID
is mainly used by provisioners, I did not want to use the same here until we can enforce proper RBAC for jobs (#16160)
I think it's definitely worth an issue. I can definitely foresee a template admin trying to cancel some hanging workspace build jobs and being surprised that they are not allowed to do so. |
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 👍 Some comments below, but nothing blocking.
This PR adds a command to cancel provisioner jobs.
I've re-used existing cancellation endpoints to ensure we keep current RBAC permissions. This revealed that template admins are only allowed to cancel TemplateVersionImport jobs, and not dry-run or workspace jobs.
This is due to:
coder/coderd/templateversions.go
Line 677 in 4872d14
And en explanation for it exists here:
coder/coderd/templateversions.go
Lines 464 to 465 in 4872d14
So I'm not sure what to make of this, is it a bug or not?
For now I've encoded the current behavior in the tests.
Fixes #16117
Updates #15084