-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
add warning when kubectl set env from secret leads to uppercase #107934
add warning when kubectl set env from secret leads to uppercase #107934
Conversation
/assign @seans3 |
/retest |
@seans3 Hey Sean, could you please take a look? Thanks |
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.
The concept of adding a warning in kubectl
when the environment variable is upper-cased is a good idea. As mentioned in the code, it is cleaner to pull this validation code into its own function. Importantly, you should add a unit test to prove the code works. Since the code outside kubectl
that you've modified is only formatting changes, you may want to remove those changes so you won't have to get reviewers from other SIG's involved.
Thanks for taking the initiative on this.
/triage accepted |
/priority important-soon |
staging/src/k8s.io/apiextensions-apiserver/third_party/forked/celopenapi/model/registry.go
Outdated
Show resolved
Hide resolved
f9edb26
to
a3122c9
Compare
48cc18c
to
6d514d1
Compare
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.
Small nit about exporting, and then we can merge. Thanks for adding tests.
6d514d1
to
feb84cc
Compare
Sorry, I may not get your point. Do you mean change If my assumption is wrong, please let me know and I will rework it :-) |
/retest |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlsong01, seans3 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 |
func (o *EnvOptions) keyToEnvName(key string) string { | ||
envName := strings.ToUpper(validEnvNameRegexp.ReplaceAllString(key, "_")) | ||
if envName != key { | ||
fmt.Fprintf(o.ErrOut, "warning: key %s transferred to %s\n", key, envName) |
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.
Do these end up rendered similarly to the way https://kubernetes.io/blog/2020/09/03/warnings/ explains? (colored yellow, and with an initial capital W)?
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.
No, it's just normal font color, without colored yellow and an Initial capital W.
The warning style exists in several places. If you search with fmt.Fprintf(o.ErrOut, "warning: xxxx
, you can find them.
Do we need coordinate them to the way as you mentioned? If so, I am glad to issue another PR to do the 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.
[cleanup]
Sounds good to me. How about first filing a kubectl issue to track the work? See https://github.com/kubernetes/kubectl/issues/new/choose
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.
Sure, track issue first :-)
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.
FYI - kubernetes/kubectl#1177
What type of PR is this?
/kind bug
What this PR does / why we need it:
Show warning when kubectl set env from secret leads to uppercase of the secret keys.
Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#1171
Special notes for your reviewer:
some go format refinement included.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: