Skip to content

Avoid adding test flags when using the null logger #82

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

Merged

Conversation

DirectXMan12
Copy link
Contributor

The null logger from logr is in the same package as the testing logger,
which means it accidentally adds testing flags to all binaries that
import it. Copy the null logger into the pkg/runtime/log package
to avoid this.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 20, 2018
@DirectXMan12 DirectXMan12 requested a review from pwittrock July 20, 2018 20:35
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 20, 2018
@DirectXMan12 DirectXMan12 requested a review from mengqiy July 20, 2018 20:36
@mengqiy
Copy link
Member

mengqiy commented Jul 23, 2018

LGTM
gometalinter is not happy. Please fix it :)

@DirectXMan12 DirectXMan12 force-pushed the bug/accidentally-testing branch from d5c3ebb to 455d0ad Compare July 23, 2018 17:45
@DirectXMan12
Copy link
Contributor Author

DirectXMan12 commented Jul 23, 2018

This should fix the lint issues. I always forget to run gometalinter, since it's not written anywhere (that I've seen). Maybe we should add it to CONTRIBUTING.md or have a makefile target for it, or something?

@mengqiy
Copy link
Member

mengqiy commented Jul 23, 2018

Yeah, we can add a section in CONTRIBUTING.md for it.

Linter is still complaining.
You can run ./test.sh to ensure everything is passing before push new changes :)

@DirectXMan12
Copy link
Contributor Author

whoops, typo. Yeah, I found test.sh (it does run all the tests though, and sometimes you just want to run the linter).

@DirectXMan12
Copy link
Contributor Author

I'll push a PR later today around that.

The null logger from logr is in the same package as the testing logger,
which means it accidentally adds testing flags to all binaries that
import it.  Copy the null logger into the pkg/runtime/log package
to avoid this.
@DirectXMan12 DirectXMan12 force-pushed the bug/accidentally-testing branch from 455d0ad to 434aee9 Compare July 24, 2018 14:36
@DirectXMan12
Copy link
Contributor Author

there, that should fix it. actually remembered to run the right linter command this time ;-)

Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2018
@mengqiy mengqiy added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2018
@k8s-ci-robot k8s-ci-robot merged commit a173375 into kubernetes-sigs:master Jul 24, 2018
@DirectXMan12 DirectXMan12 deleted the bug/accidentally-testing branch July 24, 2018 21:55
justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018
…lly-testing

Avoid adding test flags when using the null logger
DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
fixed build artifact path for gsutil copy command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants