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

WebDiscover: allow custom labels for single resource enroll (server, kube, eks, rds) #50606

Open
wants to merge 11 commits into
base: lisa/add-v2-endpoint
Choose a base branch
from

Conversation

kimlisa
Copy link
Contributor

@kimlisa kimlisa commented Dec 30, 2024

part of #46976

recommend reviewing by commit

This PR adds ability to define custom labels for single resource enroll (support for app will be in another PR)

kube story
image

server story
image

eks story
image

rds story
image

tests done on my staging cluster:

  • server
  • kube
  • single rds
  • single eks

changelog: Adds support for defining labels in the web UI for single resource enroll (server, kubernetes, EKS, and RDS)

Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I'll continue the review later today!

Copy link
Member

Choose a reason for hiding this comment

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

I think the double "Next" button might end up being confusing. 😅

Maybe the button could say "Skip adding labels" if there's no labels set and "Finish adding labels" if there's at least one label?

double-next

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i took this design from the helm chart step, b/c it does the same thing 😅. What do you think about Generate Script instead, b/c for both steps, its what it does 🤔

image

Copy link
Member

Choose a reason for hiding this comment

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

"Generate Script" / "Generate Command" would work in the case of the screenshot that you posted. Step 2 talks about generating a command, so it wouldn't be unexpected to have a button that says "Generate Command".

In the case of adding a single server, there's no mention of a command, so I feel a button that says "Generate Command" would be out of place there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, i edited the button texts

server (
image
image

kube:
image

Copy link
Member

Choose a reason for hiding this comment

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

Nice. 👍 I think it'd make sense to use title case for these since that's what we use for other buttons (e.g. "Add Another Label", "Add a Label").

@ravicious ravicious self-requested a review January 2, 2025 13:43
<ResourceLabelTooltip resourceKind="kube" />
</Flex>
<Box mb={3}>
<LabelsCreater
Copy link
Member

Choose a reason for hiding this comment

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

I can't seem to enter labels until I fill out Teleport Service Namespace and Kubernetes Cluster Name first. Would it be possible to validate this only once I click on the first "Next"?

Copy link
Member

Choose a reason for hiding this comment

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

Now I can enter labels separately from the rest of the inputs, but when I click on the first "Next", it fails with "generateScript is not a function" in the console.

Copy link
Contributor Author

@kimlisa kimlisa Jan 6, 2025

Choose a reason for hiding this comment

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

this has been fixed, tested both pnpm start-teleport-e and in my staging cluster

if interested here is the updated vite config: https://github.com/gravitational/teleport/pull/50472/files#diff-3ba1ef1723154b05797a019f68c65252cbdbafd733eff347e99575e0378085a7

@kimlisa kimlisa force-pushed the lisa/add-v2-endpoint branch 7 times, most recently from 3ed0168 to 67c5614 Compare January 3, 2025 06:15
@kimlisa kimlisa force-pushed the lisa/single-enroll-labels branch from 5634e11 to 0578489 Compare January 3, 2025 08:16
@kimlisa kimlisa requested a review from ravicious January 3, 2025 08:17
Copy link
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

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

I'm still having some issues when adding a single kube cluster.

@kimlisa kimlisa force-pushed the lisa/add-v2-endpoint branch from 67c5614 to fb39fee Compare January 3, 2025 22:44
@kimlisa kimlisa force-pushed the lisa/single-enroll-labels branch 2 times, most recently from 90dd831 to c4c83db Compare January 6, 2025 20:06
@kimlisa kimlisa requested a review from ravicious January 6, 2025 21:46
@kimlisa kimlisa force-pushed the lisa/add-v2-endpoint branch from fb39fee to bc43b78 Compare January 7, 2025 09:08
@kimlisa kimlisa force-pushed the lisa/single-enroll-labels branch from c4c83db to 1c33122 Compare January 7, 2025 09:10
Copy link
Member

Choose a reason for hiding this comment

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

Nice. 👍 I think it'd make sense to use title case for these since that's what we use for other buttons (e.g. "Add Another Label", "Add a Label").

Copy link
Member

Choose a reason for hiding this comment

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

Did you have a chance to investigate what was causing the UI to blow up when the requests weren't properly proxied by the Vite server? It makes me think that perhaps we don't account for a failing request somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants