Add details for field.Required #9198
Conversation
| } | ||
| } | ||
|
|
||
| if (len(certInfo.CertFile) > 0 && len(certInfo.KeyFile) == 0) || (len(certInfo.CertFile) == 0 && len(certInfo.KeyFile) > 0) { |
There was a problem hiding this comment.
is this (len(certInfo.CertFile) == 0) != (len(certInfo.KeyFile) == 0) ?
There was a problem hiding this comment.
Damned boolean logic. Yes, this does reduce to that. I'll tweak it.
There was a problem hiding this comment.
boolean logic can be tricky, true or false?
There was a problem hiding this comment.
False, due to the use of the phrase "can be".
There was a problem hiding this comment.
wrong, the answer is true, since true or false always evaluates to true ;)
sgallagher
force-pushed
the
field-required
branch
from
5891167 to
2355a72
Compare
|
@deads2k PTAL |
| } | ||
| if len(identity.User.Name) != 0 && len(identity.User.UID) == 0 { | ||
| allErrs = append(allErrs, field.Required(userPath.Child("uid"), "")) | ||
| allErrs = append(allErrs, field.Required(userPath.Child("uid"), "username is required when UID is provided")) |
There was a problem hiding this comment.
this message doesn't make sense... they provided a username
There was a problem hiding this comment.
this message doesn't make sense... they provided a username
still outstanding.
sgallagher
force-pushed
the
field-required
branch
from
2355a72 to
4d493d6
Compare
| @@ -155,10 +155,10 @@ func ValidateIdentity(identity *api.Identity) field.ErrorList { | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("name"), identity.User.Name, msg)) | |||
| } | |||
| if len(identity.User.Name) == 0 && len(identity.User.UID) != 0 { | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("uid"), identity.User.UID, "may not be set if user.name is empty")) | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("uid"), identity.User.UID, "username is required when UID is provided")) | |||
There was a problem hiding this comment.
something went weird here. If username is required, why not field.Required(userPath.Child("username"),....) ?
sgallagher
force-pushed
the
field-required
branch
2 times, most recently
from
72a9b84 to
810facb
Compare
|
@deads2k Looks like I goofed and pushed an old branch by mistake. This should address your comments from this morning. |
sgallagher
force-pushed
the
field-required
branch
from
810facb to
ce55c58
Compare
| @@ -155,10 +155,10 @@ func ValidateIdentity(identity *api.Identity) field.ErrorList { | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("name"), identity.User.Name, msg)) | |||
| } | |||
| if len(identity.User.Name) == 0 && len(identity.User.UID) != 0 { | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("uid"), identity.User.UID, "may not be set if user.name is empty")) | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("uid"), identity.User.UID, "username is required when username is provided")) | |||
There was a problem hiding this comment.
Phrasing this way is good, but you need to make it field.Required(userpath.Child("username")...) to match.
Some fields are only required if other fields have certain values. This patch adds some additional error messaging to inform the user why these fields were necessary. Fixes issue openshift#7118 Signed-off-by: Stephen Gallagher <[email protected]>
sgallagher
force-pushed
the
field-required
branch
from
ce55c58 to
2193b4d
Compare
| @@ -155,10 +155,10 @@ func ValidateIdentity(identity *api.Identity) field.ErrorList { | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("name"), identity.User.Name, msg)) | |||
| } | |||
| if len(identity.User.Name) == 0 && len(identity.User.UID) != 0 { | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("uid"), identity.User.UID, "may not be set if user.name is empty")) | |||
| allErrs = append(allErrs, field.Invalid(userPath.Child("username"), "username is required when uid is provided")) | |||
|
rebased and addressed the remaining feedback in #11034. Closing this in favor of the new PR. |
Some fields are only required if other fields have certain values.
This patch adds some additional error messaging to inform the user
why these fields were necessary.
Fixes issue #7118
Signed-off-by: Stephen Gallagher [email protected]
Replaces #8269
@deads2k PTAL