pkg/helm: install resources in same namespace as CR #2414
Conversation
openshift-ci-robot
added
the
size/M
| } | ||
|
|
||
| func (c namespaceClientConfig) ClientConfig() (*rest.Config, error) { | ||
| return nil, nil |
There was a problem hiding this comment.
Why it is required since always return nil and nil?
There was a problem hiding this comment.
These functions are required because ToRawKubeConfigLoader() needs to return a clientcmd.ClientConfig which is an interface that requires that all of these functions be implemented.
As far as I can tell, Helm only uses ToRawKubeConfigLoader().Namespace() and not the other functions (I'll double check this), so it should be okay for those to be no-ops. We could have those unused functions return fmt.Errorf("not implemented") if that makes sense.
There was a problem hiding this comment.
Just following up on the question of how ToRawKubeConfigLoader() is used. Here's the grep output from the Helm project:
$ grep -r ToRaw *
pkg/kube/factory.go: // ToRawKubeConfigLoader return kubeconfig loader as-is
pkg/kube/factory.go: ToRawKubeConfigLoader() clientcmd.ClientConfig
pkg/kube/client.go: if ns, _, err := c.Factory.ToRawKubeConfigLoader().Namespace(); err == nil {
pkg/cli/environment.go: if ns, _, err := s.RESTClientGetter().ToRawKubeConfigLoader().Namespace(); err == nil {So yeah, only Namespace() actually needs to be implemented.
| return c.namespace, false, nil | ||
| } | ||
|
|
||
| func (c namespaceClientConfig) ConfigAccess() clientcmd.ConfigAccess { | ||
| return nil |
|
Before we merge this, I also want to update the e2e helm test to deploy the CR in a separate namespace to prevent this regression in the future. |
openshift-ci-robot
added
size/L
joelanford
requested review from
bharathi-tenneti and
theishshah
and removed request for
AlexNPavel
|
@camilamacedo86 PTAL |
| kubectl create -f "$OPERATORDIR/deploy/crds/helm.example.com_nginxes_crd.yaml" | ||
| kubectl create -f "$OPERATORDIR/deploy/operator.yaml" | ||
| kubectl create -f "$OPERATORDIR/deploy/service_account.yaml" | ||
| kubectl create -f "$OPERATORDIR/deploy/cluster_role.yaml" |
There was a problem hiding this comment.
The addition of --namespace below makes sense, but why is the test operator now cluster-scoped? Shouldn't its manifests just have a different namespace?
There was a problem hiding this comment.
The change to the operator.yaml is to make it watch all namespaces so that it can see CRs outside of its own namespace. Not sure if that answers your question...
|
Closes #2416 |
| } | ||
| return kube.New(c), nil | ||
| } | ||
|
|
There was a problem hiding this comment.
It as unused before. 👍
This commit fixes a bug that causes all release resources to be created in the namespace that the operator is deployed in, not the namespace that the CR is deployed in.
joelanford
force-pushed
the
helm-fix-install-namespace
branch
from
025c706
to
f995a8b
Compare
|
New changes are detected. LGTM label has been removed. |
Description of the change:
This PR fixes a bug that causes all release resources
to be created in the namespace that the helm-operator is deployed
in, not the namespace that the CR is deployed in.
Motivation for the change:
Closes #2416