Skip to content
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

Add Registry UI #625

Merged
merged 12 commits into from
Jan 10, 2024
Merged

Add Registry UI #625

merged 12 commits into from
Jan 10, 2024

Conversation

CannonLock
Copy link
Contributor

  • Add Components for viewing Namespace
  • Add forms for creating and updating Namespaces
  • Update login page for CIlogin Users

- Add Components for viewing Namespace
- Add forms for creating and updating Namespaces
- Update login page for CIlogin Users
@CannonLock CannonLock requested a review from haoming29 January 9, 2024 19:41
- Fix Types
@CannonLock
Copy link
Contributor Author

@haoming29 This test failure isn't likely to be my error

@haoming29
Copy link
Contributor

@haoming29 This test failure isn't likely to be my error

Nope. That's a flaky test, will re-run it

Copy link
Contributor

@haoming29 haoming29 left a comment

Choose a reason for hiding this comment

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

We don't seem to allow no-authed users to view registry page? When I didn't login and go to https://<registry-host>/ or https://<registry-host>/view/registry/, I will be redirected to the login page.

web_ui/frontend/app/registry/page.tsx Show resolved Hide resolved
- Display error if provided in json
- Fixup, add minHeight to multiline so styles so the original presentation aligns with the other fields
Copy link
Contributor

@haoming29 haoming29 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Left suggestions as comment.

CannonLock and others added 2 commits January 9, 2024 16:08
- Fixup based on review
- Don't expand text on approve/deny
- Add the pubkey placeholder
- Add security helper text
- Remove redirect on registry
@haoming29
Copy link
Contributor

haoming29 commented Jan 9, 2024

LGTM after the revision! One last thing to add and we should be good to go. @CannonLock

Can we add some text under "Pending Registration" and "Namespace Registry" clarifying who the namespace listed below belong to? When I login as a regular user it's a bit confusing to see "Pending Registration" and "Origin" and "Cache" as I don't know if this is others' namespace or mine.

The help text can be based on the user role:

  • Under "Namespace Registry"
    • Admin users: Namespace registry shows all registered namespaces in the federation. As an admin user, you can check pending namespace registration and review them. Approved registration are listed under "Origin" or "Cache" table. Denied registrations are hidden.
    • Regular users: Namespace registry shows all registered namespaces in the federation. You can edit your registrations prior to approval. If you want to edit your registration that has been approved, please contact your registry administrator.
  • Under "Pending Registration"
    • Admin users: Pending Registration are namespaces waiting for your review and approve or deny. If a cache is in pending status, the director will not use that cache
    • Regular users: Pending Registration are namespaces waiting for admin to review. Caches with pending status will not be used in the federation until they are approved

- Add some clarifying text
- Add avatar if you own the namespace
@CannonLock
Copy link
Contributor Author

@haoming29 I made some tweaks in my changes to reduce the amount of text. It is hard to get people to read text so I tried to add some ui elements to answer your questions and your text suggestions where UI couldn't answer them all.

Sorry about missing the issue you had to fix, I thought I saw a second suggested change for the input form but I looked through a couple times and couldn't find it.

Copy link
Contributor

@haoming29 haoming29 left a comment

Choose a reason for hiding this comment

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

LGTM with the revision. Thanks for putting this together.

- Account for undefined authentication
@haoming29
Copy link
Contributor

haoming29 commented Jan 10, 2024

Looks like the only test failure is the client side test that's supposed to be fixed by @joereuss12 soon, if this is what we talked about during the standup? Will merge ours first.

@haoming29 haoming29 merged commit 87e8d86 into PelicanPlatform:main Jan 10, 2024
15 of 17 checks passed
@haoming29 haoming29 mentioned this pull request Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants