Allow dash (-) in BuildConfig env vars#20738
Allow dash (-) in BuildConfig env vars#20738openshift-merge-robot merged 1 commit intoopenshift:masterfrom coreydaley:bugzilla_1614155_allow_dash_in_bc_env_vars
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
/assign bparees |
openshift-ci-robot
added
size/S
coreydaley
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
@openshift/sig-developer-experience ptal |
|
/retest |
adambkaplan
left a comment
adambkaplan
left a comment
There was a problem hiding this comment.
Need to handle case where more than one error is returned by kvalidation
| allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, cIdentifierErrorMsg)) | ||
| } | ||
| if errs := kvalidation.IsEnvVarName(ev.Name); len(errs) != 0 { | ||
| allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, strings.Join(errs, ""))) |
There was a problem hiding this comment.
Need a delimiter here if multiple errors are returned (semicolon with a space after is my preference).
Since allErrs is a list, you can iterate over the returned errs and add a field.Invalid(...) error for each one, though this may break current conventions.
pkg/oc/util/env/env.go
Outdated
| n, v := parts[0], parts[1] | ||
| if errs := validation.IsEnvVarName(n); len(errs) != 0 { | ||
| return nil, nil, fmt.Errorf("%ss must be of the form key=value, but is %q", envVarType, envSpec) | ||
| return nil, nil, fmt.Errorf("%s %s is invalid, %s", envVarType, envSpec, strings.Join(errs, "")) |
There was a problem hiding this comment.
Need delimeter (keep consistency as above).
|
/approve |
|
/lgtm |
| allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, cIdentifierErrorMsg)) | ||
| } | ||
| if errs := kvalidation.IsEnvVarName(ev.Name); len(errs) != 0 { | ||
| allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, strings.Join(errs, "; "))) |
There was a problem hiding this comment.
why not else-if? you're just going to give someone two errors on an empty value now.
There was a problem hiding this comment.
kvalidation.IsEnvVarName would have to be run a second time to get the error message inside of the else.
There was a problem hiding this comment.
if len(env.Name) ==0 {
allErrs=
} else {
if errs:=kvalidation.IsEnvVarName(); len(errs)!=0 {
allErrs= append
}
}
There was a problem hiding this comment.
also
if a:=false; a {
fmt.Println(a)
} else if b:=true; b {
fmt.Println(b)
}
is valid syntax.
There was a problem hiding this comment.
so you don't even have to do what i wrote
…keys in the build configuration. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1614155
|
/lgtm |
openshift-ci-robot
added
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, bparees, coreydaley The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@jhadvig Can you look at updating the regex in console? |
6 participants
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1614155