Warn when secret keys are not valid env var names#2039
Warn when secret keys are not valid env var names#2039openshift-bot merged 1 commit intoopenshift:masterfrom
Conversation
| <label class="add-choice" for="mountVolume"> | ||
| <input type="radio" ng-model="ctrl.addType" value="volume" ng-disabled="ctrl.disableInputs"> | ||
| <label class="add-choice" for="volume"> | ||
| <input id="volume" type="radio" ng-model="ctrl.addType" value="volume" ng-disabled="ctrl.disableInputs"> |
There was a problem hiding this comment.
If the <label> element wraps the <input>, you don't need the for and id attributes.
There was a problem hiding this comment.
If I remove it then clicking on the label does not toggle the radio. It's hard to click on just the little radio button.
There was a problem hiding this comment.
Really? Hm. It seems to work here, but maybe browser specific?
https://getbootstrap.com/docs/3.3/css/#checkboxes-and-radios
OK to leave if it's needed.
There was a problem hiding this comment.
You're right, dunno what I was doing wrong before...
| </label> | ||
| <div class="alert alert-warning env-warning" ng-show="ctrl.invalidEnvVars.length"> | ||
| <span class="pficon pficon-warning-triangle-o"></span> | ||
| <span>Some of the keys for secret <strong>{{ctrl.secret.metadata.name}}</strong> are not valid environment variable names and will not be added.</span>. |
There was a problem hiding this comment.
Looks like you have two periods
| Environment variables | ||
| </label> | ||
| <div class="alert alert-warning env-warning" ng-show="ctrl.invalidEnvVars.length"> | ||
| <span class="pficon pficon-warning-triangle-o"></span> |
| if (!keyValidator.test(nextKey)) { | ||
| ctrl.invalidEnvVars.push(nextKey); | ||
| } | ||
| }); |
There was a problem hiding this comment.
If you only care if some key is invalid, this could be
ctrl.hasInvalidEnvVars = _.some(ctrl.secret.data, function(value, key) {
return !keyValidator.test(key);
});Otherwise, I'd probably avoid the extra keys call by using
_.each(ctrl.secret.data, function(value, key) {
if (!keyValidator.test(key)) {
ctrl.invalidEnvVars.push(key);
}
});There was a problem hiding this comment.
Yeah, I was going do do that but wasn't sure if we wanted to be able to list the invalid keys at some point. Thoughts?
There was a problem hiding this comment.
Still working on getting the validator to work correctly.
var keyValidator = new RegExp("[A-Za-z_][A-Za-z0-9_]*");
is allowing 'database-name' (as well as 'database=name')...
There was a problem hiding this comment.
Maybe
var keyValidator = new RegExp("^[A-Za-z_][A-Za-z0-9_]*$");There was a problem hiding this comment.
I believe I've got it working with:
var keyValidator = new RegExp("^[A-Za-z_]{1}[A-Za-z0-9_]{0,}$");There was a problem hiding this comment.
Yes, it is. Got carried away with some other attempts... ;)
jeff-phillips-18
force-pushed
the
secrets
branch
from
eb34522 to
381269a
Compare
|
Believe I have addressed all of @spadgett's comments. |
spadgett
left a comment
spadgett
left a comment
There was a problem hiding this comment.
LGTM, thanks @jeff-phillips-18
|
Thanks @jeff-phillips-18 the new visuals look great 👍 |
|
[merge][severity: bug] |
|
[Test]ing while waiting on the merge queue |
|
Evaluated for origin web console test up to 381269a |
|
Origin Web Console Test Results: FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/160/) (Base Commit: 375e384) (PR Branch Commit: 381269a) |
|
[merge][severity: bug] |
|
WebDriver flake [merge][severity:bug] |
|
[merge][severity:bug] |
1 similar comment
|
[merge][severity:bug] |
|
Evaluated for origin web console merge up to 381269a |
|
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/168/) (Base Commit: ca54bec) (PR Branch Commit: 381269a) (Extended Tests: bug) |
4 participants
fixes #2029
@beanh66