-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨clusterctl: add CLUSTERCTL_LOG_LEVEL environment variable #2996
Conversation
/area clusterctl |
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.
@cpanato thanks for this PR
In clusterctl flags takes precedence on env variables.
Is it possible to change this PR to comply with the above rule?
Also, in clusterctl we are allowing to use env variables or config variables by using a pluggable config component, while the management of this variable is totally detached from the rest of the config variables (e.g. this variable can't be set using the config file).
@wfernandes is this what you are expecting?
@fabriziopandini yes i can change that, also can you point to me this part of the config variables? and i will wait as well from @wfernandes for some feedback before apply any changes because it might change everything :) |
the config is managed in this package https://github.com/kubernetes-sigs/cluster-api/tree/master/cmd/clusterctl/client/config |
@fabriziopandini did one change for feedback and keep the old change in the previous commit for feedback one thing that is weird and maybe because I'm doing something wrong is i tried to print the
|
@cpanato Thanks for this PR. I'll take a look at it by EOD today. Also in the above comment, I think you intended it for @fabriziopandini instead of fabianofranz. |
Ok, figured out, the value is not available in the init function, only outside that. |
@wfernandes correct 😬 fixed |
Reviewing |
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.
This looks good to me. I agree with @fabriziopandini suggestions regarding log level priority. That is, flag value should take precedence.
The only other thing I'd like to add is documentation for this config variable. It could either be a new section in https://cluster-api.sigs.k8s.io/clusterctl/developers.html
or https://cluster-api.sigs.k8s.io/clusterctl/configuration.html
since it still could be useful to others and it pertains to clusterctl configuration.
I prefer the latter since we could just add a section about debugging/logging in clusterctl and mention the -v
flag and then also mention the env var CLUSTERCTL_LOG_LEVEL
. But I leave it up to you 🙂
@wfernandes applied the feedback changes you requested. PTAL when you have time |
/test pull-cluster-api-capd-e2e |
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.
@cpanato thanks for this new version
Looks good to me, I will test it locally ASAP and report back
55657de
to
e76f123
Compare
I ran this locally and it looks good to me. Thanks so much for doing this PR. I'm going to use the heck out of this feature 😄 I like the fact that I can set the config variable to a high number but then I can do something like /lgtm |
/lgtm |
I'll leave approval for @fabriziopandini since he mentioned that he'd like to test it out. |
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.
Looks really good. I had to get nitpicky to find anything to comment on. I wouldn't hold this commit back for any of my nits, just thought I would suggest them in case you wish to do another revision.
To have more verbose logs you can use the `-v` flag when running the `clusterctl` and set the level of the logging verbose from 0 to 5. | ||
|
||
If you do not want to use the flag every time you issue a command you can set the environment variable `CLUSTERCTL_LOG_LEVEL` or set the variable in the `clusterctl` config file which is located by default at `$HOME/.cluster-api/clusterctl.yaml`. |
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.
It may be helpful to have a link to the book at https://cluster-api.sigs.k8s.io/clusterctl/configuration.html for more details.
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.
not clear to me your comment, this piece of the documentation will be in the https://cluster-api.sigs.k8s.io/clusterctl/configuration.html page
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.
Fair point, I should have paid attention to the path of the file. 😅
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.
Two small nits and then lgtm for me as well
cmd/clusterctl/cmd/root.go
Outdated
if err == nil && v != "" { | ||
verbosityFromEnv, err := strconv.Atoi(v) | ||
if err != nil { | ||
fmt.Printf("Failed to convert CLUSTERCTL_LOG_LEVEL string to an int. err=%s", err.Error()) |
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.
Add a new line at the end and redirect to os.Stderr
.
fmt.Printf("Failed to convert CLUSTERCTL_LOG_LEVEL string to an int. err=%s", err.Error()) | |
fmt.Fprintf(os.Stderr, "Failed to convert CLUSTERCTL_LOG_LEVEL string to an int. err=%s\n", err.Error()) |
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.
done
if -v is set it will override the CLUSTERCTL_LOG_LEVEL variable.
/test pull-cluster-api-e2e |
1 similar comment
/test pull-cluster-api-e2e |
@cpanato: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpanato, fabriziopandini 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 |
What this PR does / why we need it:
add a
CLUSTERCTL_LOG_LEVEL
environment variable to be used instead of the-v
.if
CLUSTERCTL_LOG_LEVEL
is set it will override the flag-v
example, I did not have any running cluster, then I added some log output to make the example.
UPDATED with the new behavior
and
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):/kind feature
cc @wfernandes