-
Notifications
You must be signed in to change notification settings - Fork 34
Initialize klog with (go)flags and add these flags to pflag as well #71
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
Initialize klog with (go)flags and add these flags to pflag as well #71
Conversation
da812b9
to
8cba2c0
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.
@jerolimov and I did a live coding session where we experimented with other kubernetes libraries to initialize klog's flags. Unfortunately this didn't work - the functions here are the best we can do. I have added a few stylistic suggestions to make the code more "go-like".
@jerolimov I think you can also add an e2e test to verify we are adding the desired flags. We use the Bats framework for running the shp
command line as if it were run by an end user. You can an example of this in the run-follow test. The bats assert libraries will probably help as well.
/kind feature |
8cba2c0
to
c75be31
Compare
Thanks @adambkaplan for the feedback. I've updated the PR with "go style" function names and godocs, and rebased it as well. I've also added some bat e2e tests for the Please take a new look. Thanks. 🙂 The last envvars.bats tests fails for me on main and with this PR, so I comment out the last 3 asserts in envvars.bats (only locally). ➜ cli git:(init-klog) ✗ make build test-e2e
go build -v -mod=vendor -o _output/shp ./cmd/shp/...
./test/e2e/bats/core/bin/bats --recursive test/e2e/*.bats
✓ shp binary can be executed
✓ shp --help lists all available commands
✓ shp --help lists some common flags
✓ shp --help lists also logging flags
✓ shp [build/buildrun] create should not error when a name is specified
✓ shp -v=10 build list can log the kubernetes api communication
✓ shp environment variable lifecycle
✓ shp build run follow verification
8 tests, 0 failures /cc @coreydaley this PR doesn't update any output from the |
c75be31
to
4cebc0a
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.
/approve
Looks good to me.
As an FYI, there has been improvement upstream in the latest version of k8s.io/component-base that simplifies initialization of klog.
We should consider refactoring to it with our next kube rebase.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan 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 |
@adambkaplan how do we get this merged? :) |
/assign @otaviof |
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.
Good additions to the CLI, @jerolimov! Thanks.
/lgtm
Changes
Just initialize klog with go flags and add these flags to pflag as well.
Fixes #7
With this change its possible to enable k8s api communication, for example:
Help output shows
klog
attributes as well as all subcommands. See-v
and all these--log*
arguments.Submitter Checklist
See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.
Release Notes