-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
KV alert banner for white space in KV path #12921
Conversation
@@ -52,7 +52,7 @@ export default Model.extend(Validations, { | |||
defaultValue: 0, | |||
label: 'Maximum number of versions', | |||
subText: | |||
'The number of versions to keep per key. Once the number of keys exceeds the maximum number set here, the oldest version will be permanently deleted. This value applies to all keys, but a key’s metadata settings can overwrite this value.', | |||
'The number of versions to keep per key. Once the number of keys exceeds the maximum number set here, the oldest version will be permanently deleted. This value applies to all keys, but a key’s metadata settings can overwrite this value. When 0 is used or the value is unset, Vault will keep 10 versions.', |
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.
This was a quick fix on the max_version message that I confirmed with design.
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, just one minor cleanup.
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.
A couple cleanup things, otherwise looks great!
ui/app/styles/core/alert-banner.scss
Outdated
@@ -4,3 +4,7 @@ | |||
|
|||
color: $black; | |||
} | |||
|
|||
.margin-top { |
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.
In helpers.scss
we have .has-top-margin
helpers, is there a reason we have it set here specifically rather than using a helper?
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.
Good catch. I missed this. When I search the code I was looking for has-margin-top, I'll remove.
@@ -29,7 +30,7 @@ export default Component.extend({ | |||
progressBar: null, | |||
yieldWithoutColumn: false, | |||
marginless: false, | |||
classNameBindings: ['containerClass'], | |||
classNameBindings: ['containerClass', 'marginTop:margin-top'], |
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'd prefer to wrap the component in classes that adjust the layout, rather than adding the need for this component to know it has a margin top (plus, that implies the need for support of all the margins in the future)
@@ -440,7 +440,7 @@ module('Acceptance | secrets/secret/create', function(hooks) { | |||
} | |||
}); | |||
|
|||
test('create secret with space shows version data', async function(assert) { | |||
test('create secret with space shows version data and shows space warning', async function(assert) { |
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.
👏
This reverts commit ac83254.
@@ -28,6 +28,16 @@ | |||
The secret path may not end in <code>/</code> | |||
</p> | |||
{{/if}} | |||
{{#if this.pathWhiteSpaceWarning}} | |||
<div class="has-top-margin-m"> |
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.
had to add the div to add the margin, unable to add margin class to component itself.
This PR adds a warning when the user has a space of any kind in the path of a KV 2 secret.
In the screenshot the space is a trailing white space (at the end of the path name).