Define and implement command line parsing with Options struct #1984
Conversation
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
k8s-ci-robot
added
the
kind/feature
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: elevran The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/L
|
/test pull-gateway-api-inference-extension-test-e2e-main |
Signed-off-by: Etai Lev Ran <[email protected]>
|
/test pull-gateway-api-inference-extension-test-unit-main |
| zapopts := zap.Options{ | ||
| Development: true, | ||
| } | ||
| gfs := goflag.NewFlagSet("zap", goflag.ExitOnError) | ||
| opts.BindFlags(gfs) // zap expects a standard Go FlagSet and pflag.FlagSet is not compatible. | ||
| flag.CommandLine.AddGoFlagSet(gfs) | ||
| flag.Parse() | ||
| initLogging(&opts) | ||
| gfs := flag.NewFlagSet("zap", flag.ExitOnError) | ||
| zapopts.BindFlags(gfs) // zap expects a standard Go FlagSet and pflag.FlagSet is not compatible. | ||
| pflag.CommandLine.AddGoFlagSet(gfs) | ||
| opts.AddFlags(pflag.CommandLine) |
There was a problem hiding this comment.
can we move this part inside options? or is there a reason for it being here?
There was a problem hiding this comment.
maybe also the pflag.Parse itself, and then wrap it with a single options.Parse function?
There was a problem hiding this comment.
there's reason: the zap.Options are used in initLogging in main, and not accessible in runserver.
There was a problem hiding this comment.
what are your thought about moving initLogging to be called from options.Complete and then move all this to options.go file?
There was a problem hiding this comment.
I don't think runner logging belongs in options.go
I will thin if there's a way to bind the zap options so it's "hidden" in Options and not expose in runner's main, but otherwise tend to leave as is.
There was a problem hiding this comment.
I think the following would work in options.Complete()
// ensure zap log level is set - explicitly by user or from "-v"
zapLogLevelFlag := pflag.CommandLine.Lookup(ZapLogLevelFlagName)
if zapLogLevelFlag != nil && !zapLogLevelFlag.Changed { // not set explicitly
zapLevel := -1 * opts.LogVerbosity // See https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/log/zap#Options.Level
zapLevelString := fmt.Sprintf("%d", zapLevel)
if err := zapLogLevelFlag.Value.Set(zapLevelString); err != nil {
return fmt.Errorf("failed to set %q to %s: %w", ZapLogLevelFlagName, zapLevelString, err)
}
zapLogLevelFlag.Changed = true
}and then the runner code becomes:
func initLogging(opts *zap.Options) {
logger := zap.New(zap.UseFlagOptions(opts), zap.RawZapOpts(uberzap.AddCaller()))
ctrl.SetLogger(logger)
}Not sure it is justified to make this change.
…er pflag.Parse() Signed-off-by: Etai Lev Ran <[email protected]>
Signed-off-by: Etai Lev Ran <[email protected]>
k8s-ci-robot
added
size/XL
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
3 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Move EPP command line defintion and handling into an Options struct, that includes CLI flag binding and value validation.
Which issue(s) this PR fixes:
Refs #1947
Does this PR introduce a user-facing change?: