Use the correct kubeconfig when running in-cluster #437
Conversation
|
If someone wants to try this out, see https://github.com/porridge/kuttl/releases/tag/v0.14.0-kubeconfig |
|
thanks @porridge ! |
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
This does not change kuttl behaviour, since all callers of Harness.Config() do not look at the config if a non-nil error is returned. The benefit is less cognitive effort when reading this function. Signed-off-by: Marcin Owsiany <porridge@redhat.com>
Just to keep Config() more focused and not worry about details such as trying another namespace. Signed-off-by: Marcin Owsiany <porridge@redhat.com>
Signed-off-by: Marcin Owsiany <porridge@redhat.com>
|
(rebased) |
|
ok... this is next up for review. expecting is to complete (and likely merge tomorrow) |
|
@porridge just checking... does this work in cluster? a quick review shows the call could be deeper than what is shown in the PR and I will discover tomorrow |
|
disregard... reviewing the context and issues makes things clear... Dziękuję! |
|
Yes, sorry forgot to mention: I did check that log collection works now, with |
There was a problem hiding this comment.
/lgtm
left some comments... mostly celebrations to godocs and obvious fixes on err conditions. As mentioned, This solves an issue in one use case and I have concerns regarding the removal of inclusterconfig removal. Additionally the change means kubernetes.go::InClusterConfig is no longer used and can be refactored out. I'll grab that as a cleanup on another PR.
Thanks @porridge ! Ready to merge.
| @@ -227,6 +227,8 @@ func (h *Harness) RunTestEnv() (*rest.Config, error) { | |||
|
|
|||
| // Config returns the current Kubernetes configuration - either from the environment | |||
| // or from the created temporary control plane. | |||
| // As a side effect, on first successful call this method also writes a kubernetes client config file in YAML format | |||
| // to a file called "kubeconfig" in the current directory. | |||
| } | ||
| if err != nil { | ||
| return h.config, err | ||
| return nil, err |
| } | ||
| h.T.Logf("Successful connection to cluster at: %s", h.config.Host) | ||
| } | ||
|
|
||
| // The creation of the "kubeconfig" is necessary for out of cluster execution of kubectl | ||
| // The creation of the "kubeconfig" is necessary for out of cluster execution of kubectl, | ||
| // as well as in-cluster when the supplied KUBECONFIG is some *other* cluster. | ||
| f, err := os.Create("kubeconfig") |
There was a problem hiding this comment.
I wonder if we should make this a test temp file / folder... however this is consistent with existing behavior deserves it's own PR if we want it. Thanks for the Godocs! really helpful!
There was a problem hiding this comment.
Yes, I think this should be in a temporary file (and created with an appropriate umask BTW), see also this related issue.
| } | ||
|
|
||
| defer f.Close() | ||
|
|
||
| return h.config, testutils.Kubeconfig(h.config, f) | ||
| } | ||
|
|
||
| func (h *Harness) waitForFunctionalCluster() error { |
There was a problem hiding this comment.
I wonder if this should be in kubernetes.go but it's just a refactor of code in the existing file... looks good!
| @@ -253,51 +255,54 @@ func (h *Harness) Config() (*rest.Config, error) { | |||
| return nil, err | |||
| } | |||
| h.config.WarningHandler = rest.NewWarningWriter(os.Stderr, rest.WarningWriterOptions{Deduplicate: true}) | |||
| inCluster, err := testutils.InClusterConfig() | |||
There was a problem hiding this comment.
I'm most concern with this removal... but it is likely, based on code review, the issue you are looking to solve. I vaguely remember needing this to satisfy the way operator SDK scorecard uses kuttl.
Signed-off-by: Ken Sipe <kensipe@gmail.com>
|
changed my mind :) Removing the dead code for inclusterconfig makes sense to include on this PR. merging upon CI build success. |
What this PR does / why we need it:
Fixes #393