- Notifications
You must be signed in to change notification settings - Fork1k
feat: add provisioner daemon and jobs endpoints and commands#15940
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b776b1b toaf6789aCompare| ==========[version job ID]========== ====[timestamp]===== succeeded map[owner: scope:organization] template_version_import Coder | ||
| ======[workspace build job ID]====== ====[timestamp]===== succeeded map[owner: scope:organization] workspace_build Coder |
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.
It would be nice if tags were listed ink1=v1,k2=v2 format.
We can also probably "lie" a bit about the case{"owner": "", "scope": "organization"} and just pretend it's empty.
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.
Good suggestions, and I agree we should hide default owner/scope. We should probably update table formatter to change how maps are output.
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 would move "Queue" column to be the first one.
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 would move "Queue" column to be the first one.
@mtojek I'm going to create a follow-up issue for this as ordering columns is not supported in our table formatter currently. I would like to avoid us having to craft custom structs every time we want to re-order columns.
Uh oh!
There was an error while loading.Please reload this page.
cli/provisionerjobs.go Outdated
| funcconvertSlice[D,S~string](dstType []D,src []S) []D { | ||
| for_,item:=rangesrc { | ||
| dstType=append(dstType,D(item)) | ||
| } | ||
| returndstType | ||
| } |
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 throw this intoslice asToEnum 🤷♂️
Similar to ToStrings
coder/coderd/util/slice/slice.go
Lines 7 to 14 indc70114
| // ToStrings works for any type where the base type is a string. | |
| funcToStrings[T~string](a []T) []string { | |
| tmp:=make([]string,0,len(a)) | |
| for_,v:=rangea { | |
| tmp=append(tmp,string(v)) | |
| } | |
| returntmp | |
| } |
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.
Can do 👍🏻
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.
Btw, any good reason these utils undercoderd? I wouldn't move it in this PR but could make it more accessible.
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.
no good reason at all. I think it just grew over time
| Versionstring`json:"version" table:"version"` | ||
| APIVersionstring`json:"api_version" table:"api version"` | ||
| Provisioners []ProvisionerType`json:"provisioners" table:"-"` | ||
| Tagsmap[string]string`json:"tags" table:"tags"` |
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 do not love this output:map[owner: scope:organization], but I'm ok with it.
We could have the table formatter know how to handle some basic types likemap[string]string. Just leave it for this PR though.
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 had a similar thought, something human-readable and copy/paste friendly:
owner = (none)scope = organization| cmd:=r.RootCmd.Provisioners() | ||
| cmd.AddSubcommands( | ||
| r.provisionerDaemonStart(), | ||
| r.provisionerKeys(), | ||
| ) |
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 is a crafty
| ), | ||
| Handler:func(inv*serpent.Invocation)error { | ||
| varclosersclosers | ||
| deferclosers.Close() |
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.
Lol,closers was a nice thought, but never closing the closers is quite the funny bug.
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.
varlosersclosersdeferlosers.Close()
:)
| r.Route("/provisionerdaemons",func(r chi.Router) { | ||
| r.Get("/",api.provisionerDaemons) | ||
| }) |
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.
If we add filtering bykey, can we merge this with/organizations/{organization}/provisionerkeys/daemons? Or at least merge the code they both invoke? If possible, just to make sure we are consistent.
| func (api*API)provisionerKeyDaemons(rw http.ResponseWriter,r*http.Request) { |
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.
That's a good idea, I wasn't sure how much to move to AGPL vs enterprise but none of this code is critical to keep in enterprise 👍🏻
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.
Generally I thinkGET endpoints can be made AGPL, andPOST/PATCH should be enterprise.
| ifval!=nil { | ||
| v=val.Format(time.RFC3339) | ||
| } | ||
| case codersdk.NullTime: |
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.
nit: shouldn't we extract it to a separate PR?
| jobs,err:=client.OrganizationProvisionerJobs(ctx,org.ID,&codersdk.OrganizationProvisionerJobsOptions{ | ||
| Status: slice.ToStringEnums[codersdk.ProvisionerJobStatus](status), | ||
| Limit:int(limit), |
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.
nit: paging?
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 want to keep it simple for now, I realized we don't have a precedent on the CLI for paging so I didn't want to introduce it here. Granted--limit is also new, so I could remove that too but I was worried about how many entries there will be.
The original issue#15084 mentioned we should have "created at" (or rather "created after"), and I added that initially, but then I realized we don't have a precedent for filtering on time on the CLI either. (There's even a lack of serpent support there.) For this reason both pagination and created at filter have been omitted and should IMO be part of a grander plan for how to support these in a unified way on the CLI.
| ==========[version job ID]========== ====[timestamp]===== succeeded map[owner: scope:organization] template_version_import Coder | ||
| ======[workspace build job ID]====== ====[timestamp]===== succeeded map[owner: scope:organization] workspace_build Coder |
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 would move "Queue" column to be the first one.
| iferr!=nil { | ||
| return"",nil,xerrors.Errorf("open container: %w",err) | ||
| } |
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.
nit: double-checking, is it intentional?
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.
Yes, in some cases this will cause a nil pointer dereference otherwise.
| // @Param tags query object false "Provisioner tags to filter by (JSON of the form {'tag1':'value1','tag2':'value2'})" | ||
| // @Success 200 {array} codersdk.ProvisionerDaemonWithStatus | ||
| // @Router /organizations/{organization}/provisionerdaemons [get] | ||
| func (api*API)provisionerDaemons(rw http.ResponseWriter,r*http.Request) { |
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.
Is there a reason why endpoint changes and CLI go together? Alternatively, you could split the PR in half, and keep all golden files aside. Large PRs are typically a problem when somebody needs to revert them.
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.
Yes and no, for development I'm working on them in tandem, CLI requirements have and keep resulted in API changes, etc. Stacking could be a better approach though.
I don't think the golden files should be separate as that would make the feature harder to revert, but API and CLI can definitely be.
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.
It should be possible to have the DB stuff in its own PR and the CLI stuff then in a separate PR based off that. No need to have a really deep stack here IMO.
I'm fine with reviewing initially here "all-in-one".
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.
👍
| Versionstring`json:"version" table:"version"` | ||
| APIVersionstring`json:"api_version" table:"api version"` | ||
| Provisioners []ProvisionerType`json:"provisioners" table:"-"` | ||
| Tagsmap[string]string`json:"tags" table:"tags"` |
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 had a similar thought, something human-readable and copy/paste friendly:
owner = (none)scope = organizationdocs/manifest.json Outdated
| "path":"reference/cli/provisioner_keys.md" | ||
| "path":"reference/cli/provisioners_keys.md" |
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.
nit: renaming could go in a separate PR, right?
| ), | ||
| Handler:func(inv*serpent.Invocation)error { | ||
| varclosersclosers | ||
| deferclosers.Close() |
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.
varlosersclosersdeferlosers.Close()
:)
| // We may in future decide to scope provisioner daemons to organizations, so we'll keep the API | ||
| // route as is. | ||
| r.Route("/organizations/{organization}/provisionerdaemons",func(r chi.Router) { | ||
| r.Route("/organizations/{organization}/provisionerdaemons/serve",func(r chi.Router) { |
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.
nit: does this change give any special benefits? Most likely I'm missing something...
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.
We removed the other endpoints from here that were under/organizations/{organization}/provisionerdaemons and only/organizations/{organization}/provisionerdaemons/serve remains. There's no reason to keep the broader one and IIRC it also interfered with the AGPL endpoint.
| iferr!=nil { | ||
| httpapi.Write(ctx,rw,http.StatusInternalServerError, codersdk.Response{ | ||
| Message:"Internal error fetching provisioner daemons.", | ||
| Detail:err.Error(), | ||
| }) | ||
| return | ||
| } |
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.
nit: doessql.ErrNoRows need to be handled separately?
5b54ed6 to720b715CompareCo-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
720b715 to24d49daComparemafredri commentedJan 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Fixes#15190
Fixes#15191
Updates#15084