-
Notifications
You must be signed in to change notification settings - Fork 218
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
Logcheck: Separate correct usage and migration tests #247
Conversation
/cc @serathius |
hack/tools/logcheck/main.go
Outdated
@@ -27,14 +28,26 @@ import ( | |||
"golang.org/x/tools/go/analysis/singlechecker" | |||
) | |||
|
|||
var logcheckFlags = flag.NewFlagSet("", flag.ExitOnError) |
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.
Let's avoid adding more globals, could you move Analyser and flag registration to functions?
I know globals are simpler, but they create a bad pattern in code. Let me know if you need help with this.
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.
Discussion here: #247 (comment)
0683a06
to
7054173
Compare
hack/tools/logcheck/main.go
Outdated
} | ||
var ( | ||
logcheckFlags = flag.NewFlagSet("", flag.ExitOnError) | ||
allowUnstructured = logcheckFlags.Bool("allow-unstructured", false, allowUnstructuredFlagHelpText) |
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.
Please avoid using global variables.
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.
@serathius I tried finding a way to do this, but it's either really tricky or not possible.
For flags, I had to use global variable as I couldn't find a way to pass it into run()
where it's actually used.
For Analyzer, I had to make it global so that anyone can access it from outside. In our case, unit tests can't access it if it's not global and fails.
If you have suggestions on how to do it, I can add those changes to the PR.
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.
Yea, it's a little harder then I thought at the beginning.
Try this:
type config struct {
allowUnstructured bool
}
func main() {
singlechecker.Main(analyser())
}
func analyser() *analysis.Analyzer {
c := config{}
logcheckFlags := flag.NewFlagSet("", flag.ExitOnError)
logcheckFlags.BoolVar(&c.allowUnstructured, "allow-unstructured", c.allowUnstructured, `suppress error reports when unstructured logging pattern is used`)
return &analysis.Analyzer{
Name: "logcheck",
Doc: "Tool to check use of unstructured logging patterns.",
Run: func(pass *analysis.Pass) (interface{}, error) {
return run(pass, &c)
},
Flags: *logcheckFlags,
}
}
func run(pass *analysis.Pass, c *config) (interface{}, 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.
This is neat. Made the changes.
7054173
to
f4a9f2d
Compare
By default, logcheck will check for use of unstructured logging and use of incorrect structured logging pattern, and report errors. allow-unstructured flag can be passed to logcheck to suppress errors when unstructured logging is used. It is usefull in cases when we want to test for correct use of structured logging patterns but not enfore it. Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>
Signed-off-by: Umanga Chapagain <chapagainumanga@gmail.com>
f4a9f2d
to
d6ef17a
Compare
/lgtm |
/assign @brancz |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, serathius, umangachapagain 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 |
Logcheck: Separate correct usage and migration tests
Logcheck: Separate correct usage and migration tests
What this PR does / why we need it:
logcheck: add flag for migration test
Migration test
is done by explicitly passing-migration
flag tologcheck. It runs correctness test but reports an error when
unstructured logging function is used in a migrated package.
Correctness test
is run by default to check for correct usageof structured logging patterns. It reports an error if structured
logging function is used incorrectly. It does not report an error
when structured logging function is not used.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes kubernetes/kubernetes/issues/102439
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: