Add Kubernetes access section to the role editor #47674
Conversation
bl-nero
added
the
no-changelog
api/types/constants.go
Outdated
| // is for the latest version of Role resources; roles whose version is set to | ||
| // v6 or prior only support [KindKubePod]. | ||
| // This list needs to be kept in sync with `kubernetesResourceKindOptions` in | ||
| // `web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts`. |
There was a problem hiding this comment.
I don't think we should be referring to stuff from web here, as the api package is something that we publish and that 3rd parties can use. The mention of the list in standardmodel.ts won't make a lot of sense for them. https://pkg.go.dev/github.com/gravitational/teleport/[email protected]/types#KubernetesResourcesKinds
OTOH if there's no such comment, the lists are guaranteed to desync over time. I did a quick google to see if there's a way to exclude a comment from a godoc, but I'm not sure if this is possible. If that's the case, then I think it'd be best to remove it to keeps the docs clean.
There was a problem hiding this comment.
I simply separated this to a separate comment above the godoc one.
| ? 'Add Another Resource' | ||
| : 'Add a Resource'} |
There was a problem hiding this comment.
This is inconsistent with "Add another Label" from <LabelsInput>. Though I assume <LabelsInput> is at fault here?
The designs in Figma are inconsistent as well, it uses "Add More" for labels and "Add another resource".
Personally I really dislike the titleized version but I assume the design team already has a favorite. Based on other designs in the Figma file you linked, I assume it's the titleized version.
There was a problem hiding this comment.
Yes, you're correct that the labels input is "at fault" here. The thing is, we historically had these inconsistencies pretty much everywhere, so I decided to cut it short when pushing through the change where we got rid of all-caps buttons, and documented it as a practice to be followed. At least Kenny is on board with this rule, so I guess that these inconsistencies in the design are purely incidental.
| /> | ||
|
|
||
| <Flex flexDirection="column" gap={3} mt={3}> | ||
| {value.resources.map((resource, iRes) => ( |
There was a problem hiding this comment.
Nit, but why iRes and not index or resourceIndex? When I saw resources.map((resource, iRes) => …, I assumed that resources is a map because I didn't quite understand at first what iRes is.
| onRemove={() => | ||
| onChange?.({ | ||
| ...value, | ||
| resources: value.resources.toSpliced(iRes, 1), |
There was a problem hiding this comment.
Oh nice, toSpliced is pretty cool, it might be the first time I see it. It caniuse.com says that Chrome for Android got support for it just in September this year, but I guess it should be safe? I imagine that most of the time Chrome is updated separately from the OS.
I looked up webassets/teleport/app/app.js and it doesn't look like this gets compiled to a polyfilled version, it's just raw in there.
There was a problem hiding this comment.
@ravicious No, for some reason, caniuse.com only shows the last Android Chrome version (it's even trickling down as a bug to other projects). toSpliced() has been there since version 110, which was released in February 2023 (see MDN).
That being said, if mobile browsers are on our compatibility list, it should be documented in the readme. Let me know if you have a more complete list of targets.
| <Flex flexDirection="column" gap={3} mt={3}> | ||
| {value.resources.map((resource, iRes) => ( | ||
| <KubernetesResourceView | ||
| key={iRes} |
There was a problem hiding this comment.
Using indexes as keys is not recommended (see also Keys and Reconciliation), this is particularly true for this form as you can remove an item at any position from the model.
Would it be possible to add a uuid field to KubernetesResourceModel, generate it in newKubernetesResourceModel and then make sure it doesn't make its way to the backend? I haven't looked at the code too closely yet, but I assume at some point we need to map values from the UI to some kind of yaml.
There was a problem hiding this comment.
I suppose the uuid field could be just ignored within roleEditorModelToRole?
There was a problem hiding this comment.
Oh, crap. I'm well aware that this is an anti-pattern (as a matter of fact, I was commenting on the similar thing in another PR), but somehow still fell into this trap here. :) Yes, I'll add an ID to the editor model.
| // TODO(bl-nero): This should ideally use tonal neutral 1 from the opposite | ||
| // theme as background. |
There was a problem hiding this comment.
Does the BBLP theme have an opposite theme? I wonder if each theme should have the concept of an inverse colors in itself.
There was a problem hiding this comment.
@ravicious We decided with Kenny that I'll later cherry-pick tonal neutral colors from each theme the inverse theme, treating light as inverse of BBLP. (Cherry-picking is the only option, since we can't just switch the color theme entirely for a subtree of components — exactly for the reason you described.) This should be a good solution until Kenny provides some more concrete guidelines on the Mark component itself.
To turn on the new UI, go to developer tools -> Application -> Local storage and add a
grv_teleport_use_new_role_editorkey set totrue. This will be lifted once UI is good to be released.Figma mocks
Deviations from the design:
H4instead ofH3for the resources header