Skip to content

feat: display provisioner jobs and daemons for an organization #16532

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Feb 18, 2025
Merged

feat: display provisioner jobs and daemons for an organization #16532

merged 22 commits into from
Feb 18, 2025

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma commented Feb 11, 2025 •
edited by matifali
Loading

Jobs:
Screenshot 2025-02-13 at 09 26 31
Figma Link

Daemons:
Screenshot 2025-02-13 at 09 26 53
Figma Link

Closes #15192 and Closes #15193

@BrunoQuaresma
Set base structure to display the provisioner jobs
b6430bb
@BrunoQuaresma
Merge branch 'main' of https://github.com/coder/coder into bq/refacto…
5d7d58f 
…r-provisioners
@BrunoQuaresma
[WIP]: Load data and display them in the table
643c362
@BrunoQuaresma
Merge branch 'main' of https://github.com/coder/coder into bq/refacto…
f9db209 
…r-provisioners
@BrunoQuaresma
Update table to use API data
6e967f1
@BrunoQuaresma
Finish job structure
943b7d7
@BrunoQuaresma
Display tiny alert for error
2bc6ccf
@BrunoQuaresma
Fix tags
71f4fe5
@BrunoQuaresma
Add daemons page
f027485
@BrunoQuaresma
Merge branch 'main' of https://github.com/coder/coder into bq/refacto…
dcf8140 
…r-provisioners
@BrunoQuaresma
Merge branch 'main' of https://github.com/coder/coder into bq/refacto…
994e186 
…r-provisioners
@BrunoQuaresma
Merge branch 'main' of https://github.com/coder/coder into bq/refacto…
3083cef 
…r-provisioners
@BrunoQuaresma
Display all daemon data from server
d66141e
@BrunoQuaresma
Remove unused imports
49a7ec7
@BrunoQuaresma BrunoQuaresma self-assigned this Feb 11, 2025
@BrunoQuaresma
Copy link
Collaborator Author

@jaaydenh I would appreciate a first review on this PR.

@jaaydenh
Copy link
Contributor

@BrunoQuaresma can you include in the description the issue this PR is resolving? Trying to understand the reason for the name change OrganizationProvisionersPage to just ProvisionersPage? Is the provisioners page now meant to be used outside organizations as well?

@BrunoQuaresma
Copy link
Collaborator Author

@BrunoQuaresma can you include in the description the issue this PR is resolving? Trying to understand the reason for the name change OrganizationProvisionersPage to just ProvisionersPage? Is the provisioners page now meant to be used outside organizations as well?

  • Added ✅
  • I just found the name quite redundant since it is inside of the OrganizationSettings folder but I can add the prefix back

@jaaydenh
Copy link
Contributor

Ya, the issue is that most of the components have the Organization Prefix so its more confusing if its only removed for some of them.

@BrunoQuaresma BrunoQuaresma requested a review from a team February 13, 2025 12:17
@BrunoQuaresma
Copy link
Collaborator Author

Ya, the issue is that most of the components have the Organization Prefix so its more confusing if its only removed for some of them.

Not sure if that is true.IdpSyncPage and CustomRolesPage don't use the organization prefix.

Screenshot 2025-02-13 at 09 19 52

@mafredri mafredri mentioned this pull request Feb 13, 2025
Merged
mafredri added a commit that referenced this pull request Feb 13, 2025
@mafredri
feat(coderd): add support for provisioner job id and tag filter (#16556)
e38bd27 
This change adds to new filters to the provisionerjobs endpoint, id
(array) and tags (map).

Updates #15084
Updates #15192
Related #16532
@mafredri mafredri mentioned this pull request Feb 13, 2025
Merged
Parkreiner
Parkreiner reviewed Feb 13, 2025
View reviewed changes
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Didn't find anything major here – just had a few recommendations. Not approving because I don't know how much more needs to be added to get the PR out of the draft stage, but I can do another pass once the PR goes live

site/src/pages/OrganizationSettingsPage/ProvisionersPage/CancelJobButton.stories.tsx
Comment on lines +32 to +34
parameters: {
chromatic: { disableSnapshot: true },
},
Copy link
Member

Choose a reason for hiding this comment

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

Is this what we're doing for any frontend testing that's more involved? Wondering if React Testing Library still ever makes sense for our components, or if we can offload everything to Storybook

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this what we're doing for any frontend testing that's more involved?

In this case, I only want to test the behavior since a snapshot is not valuable at all. I'm guessing disabling unnecessary snapshots can make our test suite faster.

Wondering if React Testing Library still ever makes sense for our components, or if we can offload everything to Storybook

With interaction tests, I think we can offload everything to Storybook - just a guess, but I think storybook uses testing library under the hood.

@jaaydenh
Copy link
Contributor

@BrunoQuaresma can you include in the description the issue this PR is resolving? Trying to understand the reason for the name change OrganizationProvisionersPage to just ProvisionersPage? Is the provisioners page now meant to be used outside organizations as well?

  • Added ✅
  • I just found the name quite redundant since it is inside of the OrganizationSettings folder but I can add the prefix back

Actually looking at this again, I agree it does make more sense to move the Provisioners files to their own folder and removing the prefix.

mafredri added a commit that referenced this pull request Feb 14, 2025
@mafredri
feat(coderd): add filters and fix template for provisioner daemons (#…
77306f3 
…16558)

This change adds provisioner daemon ID filter to the provisioner daemons
endpoint, and also implements the limiting to 50 results.

Test coverage is greatly improved and template information for jobs
associated to the daemon was also fixed.

Updates #15084
Updates #15192
Related #16532
@BrunoQuaresma BrunoQuaresma marked this pull request as ready for review February 14, 2025 17:19
@BrunoQuaresma
Copy link
Collaborator Author

@Parkreiner @jaaydenh I'm planning to add the storybook tests for the pages after getting this merged on main and having a QA from Christin and Bartek so I can avoid wasting time on testing something that is not what we want.

@mtojek mtojek self-requested a review February 18, 2025 13:22
mtojek
mtojek approved these changes Feb 18, 2025
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

LGTM. Let's play with it in dogfood!

Nice job, Bruno 👍

site/src/pages/OrganizationSettingsPage/OrganizationProvisionersPage.tsx
@@ -1,48 +0,0 @@
import { buildInfo } from "api/queries/buildInfo";
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I'm missing some context, could you explain why are we removing these pages? Will they be replaced now?

PS. It would be convenient to add Changes: to the PR description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they will be replaced.

@BrunoQuaresma BrunoQuaresma merged commit 0f3858e into main Feb 18, 2025
30 checks passed
@BrunoQuaresma BrunoQuaresma deleted the bq/refactor-provisioners branch February 18, 2025 14:27
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mtojek mtojek mtojek approved these changes

@jaaydenh jaaydenh Awaiting requested review from jaaydenh

@Parkreiner Parkreiner Awaiting requested review from Parkreiner

Assignees

@BrunoQuaresma BrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

provide an easy way to cancel provisioner jobs from the list create UI to show provisioner jobs
4 participants
@BrunoQuaresma @jaaydenh @mtojek @Parkreiner