BUGFIX: re-enable install test and fix flags which were not parsed correctly #14
Conversation
| @@ -31,15 +31,12 @@ import ( | |||
| cmclient "github.com/cert-manager/cert-manager/pkg/client/clientset/versioned" | |||
| ) | |||
|
|
|||
| var ( | |||
| kubeConfigFlags = genericclioptions.NewConfigFlags(true) | |||
There was a problem hiding this comment.
I made these global variables into local variables, so the test can parse flags multiple times without issues.
| } | ||
| } | ||
| } | ||
|
|
||
| func (n *NormalisedEnvSettings) InitActionConfiguration() error { | ||
| return n.ActionConfiguration.Init( | ||
| n.Factory.RESTClientGetter, | ||
| n.EnvSettings.Namespace(), | ||
| n.Factory.Namespace, |
There was a problem hiding this comment.
The n.Factory.Namespace seems like a better source-of-truth to get the namespace.
…rrectly Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
|
/lgtm |
|
@ThatsMrTalbot: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this:
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. |
| // WARNING: Do not set PreRun on the command as cobra will not execute PreRun when | ||
| // PreRunE is set. |
There was a problem hiding this comment.
I only figured this out after reading their source-code.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon 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 |
The install test was skipped because the file name did not end with
_test.go.Additionally, the flag parsing for the install and uninstall command was not working for the test because a mix of
PreRunandPreRunEwas used (cobra only runsPreRunORPreRunE).Lastly, the flags were parsed into a global variable, which does not play nicely with a test that calls the flag-parsing code multiple times.
The error was introduced in cert-manager/cert-manager#6562.