-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
UI: Add copy button for secret path #28629
UI: Add copy button for secret path #28629
Conversation
CI Results: |
Build Results: |
@@ -35,7 +38,7 @@ | |||
{{/if}} | |||
|
|||
{{#if (or (has-block "toolbarFilters") (has-block "toolbarActions"))}} | |||
<Toolbar aria-label="menu items for managing {{or @mountName @pageTitle}}"> | |||
<Toolbar aria-label="menu items for managing {{or @mountName @secretPath @pageTitle}}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern here would be, if in the future, someone uses this component with both a secretPath
and a pageTitle
the behavior would be unexpected. It seems to me that the design of this is that they are mutually exclusive: you can't have both, but only one. I checked the code and that seems to be the case.
Some thoughts:
- Create a kv-page-header.js file to have an assertion that you can't have both assigned.
- You could just stick with one property but then this else if block doesn't work.
- ... 🤔 maybe it's not a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I like option 1 because it also allows us to add documentation. But you've already gone above and beyond for a simple request so I'll make this comment non-blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking about this!
This is just for the aria-label and so it follows the same logic flow for the header above, since they're mutually exclusive and you can only have one header text 😅 . I think the conditional above makes that pretty clear 🤔 I feel like a component and documentation would be overly verbose for such a simple component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great cleanup and test coverage. A non-blocking comment.
Description
Adds a copy button for secret paths in both kv v2 and v1 secrets engines
TODO only if you're a HashiCorp employee
to N, N-1, and N-2, using the
backport/ent/x.x.x+ent
labels. If this PR is in the CE repo, you should only backport to N, using thebackport/x.x.x
label, not the enterprise labels.of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.