-
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 subkey request to kv v2 adapter #27804
UI: add subkey request to kv v2 adapter #27804
Conversation
CI Results: |
Build Results: |
@@ -10,7 +10,7 @@ | |||
data-test-overview-card-container={{@cardTitle}} | |||
...attributes | |||
> | |||
<div class="flex row-wrap space-between has-bottom-margin-m" data-test-overview-card={{@cardTitle}}> |
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.
@@ -10,7 +10,7 @@ | |||
data-test-overview-card-container={{@cardTitle}} | |||
...attributes | |||
> | |||
<div class="flex row-wrap space-between has-bottom-margin-m" data-test-overview-card={{@cardTitle}}> | |||
<div class="flex row-wrap space-between has-bottom-margin-s" data-test-overview-card={{@cardTitle}}> |
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.
Copyright (c) HashiCorp, Inc. | ||
SPDX-License-Identifier: BUSL-1.1 | ||
~}} | ||
|
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.
@@ -98,6 +99,10 @@ | |||
margin: $spacing-4 0; | |||
} | |||
|
|||
.has-top-bottom-margin-12 { |
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.
since this is a new class, I opted to use the updated -12
number naming which is a pattern we had discussed moving towards (and away from s
, m
, l
) happy to revert and make this -s
if folks prefer
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.
Can you just add a comment here to that affect? That this is the pattern we want to move toward, away from s/m/l so that future code-browsers will not be confused by the different types of measurements
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.
Yes, good idea! Will add
{{/if}} | ||
|
||
{{! Use the "subtext" yield for stylized subtext or including elements like doc links. }} | ||
{{#if (has-block "subtext")}} |
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 didn't want to update @subText
for all OverviewCards
, I felt like it made sense to keep both potential use cases. 1) just text subtext and 2) more stylized content, such as inline links
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.
Are @subtext
and <:subtext>
mutually exclusive? Right now passing both with yield both
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 intentionally made them not mutually exclusive as I didn't see a reason for them to be. Someone could add subtext in the default styling and then add a second stylized block. What do you think?
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.
Makes sense to me! I think the most confusing part is that they are named the same thing, so it felt like maybe they should be mutually exclusive. But, naming is hard 🤷
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.
looks good. one open question about putting the subkey request in the data adapter, but 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.
🚀
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.
Nice work! Just a couple non-blocking comments
ui/app/adapters/kv/data.js
Outdated
// TODO use query-param-string util when https://github.com/hashicorp/vault/pull/27455 is merged | ||
fetchSubkeys(query) { | ||
const { backend, path, version, depth } = query; | ||
const apiPath = buildKvPath(backend, path, 'subkeys'); // encodes mount and secret paths |
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.
Any reason you didn't export a kvSubkeyPath
method instead of using the base helper? Then the version and query params logic could be in there as well, and easy to unit test
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.
Since this endpoint isn't implemented yet, it was easier for my brain to fiddle with and update the logic in one place. I can move this into a kvSubkeyPath
helper. The adapter unit test felt sufficient for testing, but I can add another test for the util.
Also the default args for this endpoint work a little differently since we always send a depth
param but not always a version param, so I can't use the version logic out of the box from buildKvPath
@@ -98,6 +99,10 @@ | |||
margin: $spacing-4 0; | |||
} | |||
|
|||
.has-top-bottom-margin-12 { |
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.
Can you just add a comment here to that affect? That this is the pattern we want to move toward, away from s/m/l so that future code-browsers will not be confused by the different types of measurements
{{/if}} | ||
|
||
{{! Use the "subtext" yield for stylized subtext or including elements like doc links. }} | ||
{{#if (has-block "subtext")}} |
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.
Are @subtext
and <:subtext>
mutually exclusive? Right now passing both with yield both
Description
Adds the subkey component and request to the kv data adapter. There are no user facing changes in this PR.
✅ enterprise tests
JSON off (default) 📸
JSON on 📸