Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: add provisioner daemon name to provisioner jobs responses#17877

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

Merged

Conversation

ssncferreira
Copy link
Contributor

@ssncferreirassncferreira commentedMay 16, 2025•
edited
Loading

Description

This PR adds theworker_name field to the provisioner jobs endpoint.

To achieve this, the following SQL query was updated:

  • GetProvisionerJobsByOrganizationAndStatusWithQueuePositionAndProvisioner

As a result, thecodersdk.ProvisionerJob type, which represents the provisioner job API response, was modified to include the new field.

Notes:

  • As mentioned incomment, theGetProvisionerJobsByIDsWithQueuePosition query was not changed due to load concerns. This means that for template and template version endpoints,worker_id will still be returned, butworker_name will not.
  • Similar toworker_id, theworker_name is only present once a job is assigned to a provisioner daemon. For jobs in a pending state (not yet assigned), neitherworker_id norworker_name will be returned.

Affected Endpoints

  • /organizations/{organization}/provisionerjobs
  • /organizations/{organization}/provisionerjobs/{job}

Testing

  • Added new tests verifying that bothworker_id andworker_name are returned once a provisioner job reaches thesucceeded state.
  • Existing tests covering state transitions and other logic remain unchanged, as they test different scenarios.

Front-end Changes

Admin provisioner jobs dashboard:
Screenshot 2025-05-16 at 11 51 33

Fixes:#16982

@ssncferreirassncferreira changed the titlefeat(coderd): add provisioner daemon name to provisioner jobs responsesfeat: add provisioner daemon name to provisioner jobs responsesMay 16, 2025
@ssncferreirassncferreiraforce-pushed thessncferreira/feat-add-provisioner-daemon-name branch from8bb4c7f to5fc39a0CompareMay 16, 2025 10:39
@ssncferreirassncferreira marked this pull request as ready for reviewMay 16, 2025 11:00
Copy link
Member

@johnstcnjohnstcn left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think we're also missing some changes tocliui.ProvisionerJob and friends. If I'm understanding correctly, the provisioner name will now be available in these responses and therefore can be shown in the CLI after the "Queued" message.

EDIT: if we're just keeping the scope to showing the name in the provisioner jobs page, you can disregard what I said above.

ssncferreira reacted with eyes emoji
@BrunoQuaresma
Copy link
Collaborator

BrunoQuaresma commentedMay 18, 2025•
edited
Loading

Related to the design, and the UI, we want to remove the ID and just use the name as@ssncferreira posted on the description screenshot. No need to keep using the ID. If at some point, we have customers asking for the ID, we can think on adding it.

@johnstcn
Copy link
Member

Related to the design, and the UI, we want to remove the ID and just use the name as@ssncferreira posted on the description screenshot. No need to keep using the ID. If at some point, we have customers asking for the ID, we can think on adding it.

Why do we want to remove the ID? Keeping it on the page allows you to Ctrl+F quickly if you already know an ID.

@BrunoQuaresma
Copy link
Collaborator

@johnstcn what is the use case where the user will know the ID and not the name? Eg. We don't show the workspace name because it is more likely to the user know the name than the ID.

We had this conversation with Steven and Kayla, and the thing was, users were debugging using the name and not IDs.

ssncferreira reacted with thumbs up emoji

@johnstcn
Copy link
Member

@johnstcn what is the use case where the user will know the ID and not the name? Eg. We don't show the workspace name because it is more likely to the user know the name than the ID.

We had this conversation with Steven and Kayla, and the thing was, users were debugging using the name and not IDs.

My thought was that an admin would have an ID copied from a log or error message. If your research shows that users debug using name and not ID, then you have better data than I do!

Copy link
Member

@mafredrimafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some small suggestions, but otherwise LGTM! Nice work!

@@ -120,7 +120,7 @@ export const JobRow: FC<JobRowProps> = ({ job, defaultIsOpen = false }) => {
<>
<dt>Completed by provisioner:</dt>
<dd className="flex items-center gap-2">
<span>{job.worker_id}</span>
<span>{job.worker_name}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Worker name could be empty here if the job is old enough and the provisioner daemon has been cleaned from the DB. We could have some kind of fallback like[removed].

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good point! I’ll update it to include a fallback. That said, this raises a question, we still show the “View provisioner” button, which links to the provisioner daemon’s page. If the daemon has been cleaned from the DB, we should avoid showing the button, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Oh yeah, good catch. Definitely hide/disable the button as well 👍🏻

ssncferreira reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Handled in commit:caed396
@BrunoQuaresma let me know if this change is ok 🙂

Copy link
Member

@johnstcnjohnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM once the issues@mafredri highlighted are resolved!

@ssncferreirassncferreira merged commitf044cc3 intomainMay 19, 2025
37 of 39 checks passed
@ssncferreirassncferreira deleted the ssncferreira/feat-add-provisioner-daemon-name branchMay 19, 2025 15:05
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 19, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@johnstcnjohnstcnjohnstcn approved these changes

@mafredrimafredrimafredri approved these changes

@BrunoQuaresmaBrunoQuaresmaAwaiting requested review from BrunoQuaresma

Assignees

@ssncferreirassncferreira

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add provisioner name in the provisioner job response
4 participants
@ssncferreira@BrunoQuaresma@johnstcn@mafredri
[8]ページ先頭
©2009-2025 Movatter.jp