Allow dash (-) in BuildConfig env vars #20738
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 |
| 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
| @@ -59,7 +59,7 @@ func parseIntoEnvVar(spec []string, defaultReader io.Reader, envVarType string) | |||
| parts := strings.SplitN(envSpec, "=", 2) | |||
| 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
Needs 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? |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1614155