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: 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.

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
BrunoQuaresma merged 22 commits intomainfrombq/refactor-provisioners
Feb 18, 2025

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma commentedFeb 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 andCloses#15193

@BrunoQuaresma
Copy link
CollaboratorAuthor

@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 reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

@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.

@BrunoQuaresmaBrunoQuaresma requested a review froma teamFebruary 13, 2025 12:17
@BrunoQuaresma
Copy link
CollaboratorAuthor

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 andCustomRolesPage don't use the organization prefix.

Screenshot 2025-02-13 at 09 19 52
matifali reacted with laugh emoji

mafredri added a commit that referenced this pull requestFeb 13, 2025
This change adds to new filters to the provisionerjobs endpoint, id(array) and tags (map).Updates#15084Updates#15192Related#16532
Copy link
Member

@ParkreinerParkreiner 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

BrunoQuaresma reacted with heart emoji
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
CollaboratorAuthor

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.

BrunoQuaresma reacted with thumbs up emoji

mafredri added a commit that referenced this pull requestFeb 14, 2025
…16558)This change adds provisioner daemon ID filter to the provisioner daemonsendpoint, and also implements the limiting to 50 results.Test coverage is greatly improved and template information for jobsassociated to the daemon was also fixed.Updates#15084Updates#15192Related#16532
@BrunoQuaresmaBrunoQuaresma marked this pull request as ready for reviewFebruary 14, 2025 17:19
@BrunoQuaresma
Copy link
CollaboratorAuthor

@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.

Copy link
Member

@mtojekmtojek 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 👍

@@ -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 addChanges: to the PR description.

Copy link
CollaboratorAuthor

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.

@BrunoQuaresmaBrunoQuaresma merged commit0f3858e intomainFeb 18, 2025
30 checks passed
@BrunoQuaresmaBrunoQuaresma deleted the bq/refactor-provisioners branchFebruary 18, 2025 14:27
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsFeb 18, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@mtojekmtojekmtojek approved these changes

@jaaydenhjaaydenhAwaiting requested review from jaaydenh

@ParkreinerParkreinerAwaiting requested review from Parkreiner

Assignees

@BrunoQuaresmaBrunoQuaresma

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
[8]ページ先頭
©2009-2025 Movatter.jp