Skip to content
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

Customer requests that kubeapps do not set TargetNamespace field on CRs created by flux plugin #4189

Closed
gfichtenholt opened this issue Jan 31, 2022 · 2 comments · Fixed by #4224
Assignees
Labels
component/apis-server Issue related to kubeapps api-server kind/bug An issue that reports a defect in an existing feature

Comments

@gfichtenholt
Copy link
Contributor

gfichtenholt commented Jan 31, 2022

Discussion carried over from: #4173
In short:
Customer is requesting when kubeapps is creating new Flux HelmRelease CRs:

  1. we do not set the TargetNamespace field
  2. we set the ReleaseName field

FWIW, this is what Flux CLI does by default:

$ flux create source helm podinfo \
>     --url https://stefanprodan.github.io/podinfo \
>     --namespace default
✚ generating HelmRepository source
► applying HelmRepository source
✔ source created
◎ waiting for HelmRepository source reconciliation
✔ HelmRepository source reconciliation completed
✔ fetched revision: 83a3c595163a6ff0333e0154c790383b5be441b9db632cb36da11db1c4ece111
$ flux create hr my-podinfo \
>     --namespace=default \
>     --source=HelmRepository/podinfo.default \
>     --chart=podinfo
✚ generating HelmRelease
► applying HelmRelease
✔ HelmRelease created
◎ waiting for HelmRelease reconciliation
✔ HelmRelease my-podinfo is ready
✔ applied revision 6.0.3
$ kubectl get helmrelease/my-podinfo -o yaml
apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
creationTimestamp: "2022-01-31T07:50:56Z"
finalizers:
- finalizers.fluxcd.io
generation: 1
name: my-podinfo
namespace: default
resourceVersion: "2363"
uid: 24731219-6d27-422b-8f9a-2f71da306882
spec:
chart:
  spec:
    chart: podinfo
    reconcileStrategy: ChartVersion
    sourceRef:
      kind: HelmRepository
      name: podinfo
      namespace: default
    version: '*'
interval: 1m0s
status:
conditions:
- lastTransitionTime: "2022-01-31T07:50:57Z"
  message: Release reconciliation succeeded
  reason: ReconciliationSucceeded
  status: "True"
  type: Ready
- lastTransitionTime: "2022-01-31T07:50:57Z"
  message: Helm install succeeded
  reason: InstallSucceeded
  status: "True"
  type: Released
helmChart: default/default-my-podinfo
lastAppliedRevision: 6.0.3
lastAttemptedRevision: 6.0.3
lastAttemptedValuesChecksum: da39a3ee5e6b4b0d3255bfef95601890afd80709
lastReleaseRevision: 1
observedGeneration: 1

So:

  1. TargetNamespace field is NOT set.
    
  2. neither is ReleaseName field
    

This doesn't mean we have to do exactly the same thing

@gfichtenholt gfichtenholt changed the title Customer requests that kubeapps do not set target Customer requests that kubeapps do not set TargetNamespace field but set ReleaseName field on CRs created by flux plugin Jan 31, 2022
@gfichtenholt gfichtenholt self-assigned this Jan 31, 2022
@ppbaena ppbaena added this to the Pluggable support for Flux v2 milestone Jan 31, 2022
@ppbaena ppbaena added component/apis-server Issue related to kubeapps api-server kind/bug An issue that reports a defect in an existing feature priority/high labels Jan 31, 2022
@absoludity
Copy link
Contributor

absoludity commented Feb 2, 2022

Discussion carried over from: #4173 In short: Customer is requesting when kubeapps is creating new Flux HelmRelease CRs:

1. we do not set the TargetNamespace field

2. we set the ReleaseName field

I think what was requested (and what I was also expecting) is that we don't set either of those in the HelmRelease resource. The only reason we talked about setting the ReleaseName explicitly in #4173 was as another way to ensure that, from the end-user perspective, the actual helm release name is what you'd expect (ie. the same name as the helm release which is the name specified in the UI, but this happens by default if neither TargetNamespace nor ReleaseName is set on the HelmRelease.

FWIW, this is what Flux CLI does by default:
...

So:

1. ```
   TargetNamespace field is NOT set.
   ```

2. ```
   neither is ReleaseName field
   ```

Yep - which is what I expected us to do as well. Importantly, it results in an actual helm release (displayed with helm ls) with a name matching the HelmRelease.metadata.name, which is what's expected (at least, until our UI later provides options enabling users to specify a ReleaseName or a TargetNamespace, but I don't see that being required in the mid-term at all).

This doesn't mean we have to do exactly the same thing

I don't see any reason not to do the same thing, as the behaviour would match the expectation of creating an installed package in the Kubeapps UI called "my-apache" and finding that kubectl get helmrelease shows a helm release with the name "my-apache", and helm ls shows a helm release with the name "my-apache".

This would be something like the suggestion I left at #4204 (comment) , which only prefixes the name used for the actual helm release with the target namespace if the target namespace field was explicitly set on the HelmRelease (and IMO, targetNamespace should only be explicitly set on the HelmRelease if and when our UI provides a way to explicitly set it).

@gfichtenholt
Copy link
Contributor Author

I think what was requested (and what I was also expecting) is that we don't set either of those in the HelmRelease resource. The only reason we talked about setting the ReleaseName explicitly in #4173 was as another way to ensure that, from the end-user perspective, the actual helm release name is what you'd expect (ie. the same name as the helm release which is the name specified in the UI, but this happens by default if neither TargetNamespace nor ReleaseName is set on the HelmRelease.

yes, indeed. I went back and re-read @hongkunyoo 's comment and that is what was said. Apologies, I will correct the title

@gfichtenholt gfichtenholt changed the title Customer requests that kubeapps do not set TargetNamespace field but set ReleaseName field on CRs created by flux plugin Customer requests that kubeapps do not set TargetNamespace field on CRs created by flux plugin Feb 2, 2022
gfichtenholt added a commit to gfichtenholt/kubeapps that referenced this issue Feb 3, 2022
 Customer requests that kubeapps do not set TargetNamespace field on CRs created by flux plugin vmware-tanzu#4189
gfichtenholt added a commit that referenced this issue Feb 3, 2022
…eld on CRs created by flux plugin #4189  (#4224)

* attempt #2

* fix for #4189
 Customer requests that kubeapps do not set TargetNamespace field on CRs created by flux plugin #4189
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/apis-server Issue related to kubeapps api-server kind/bug An issue that reports a defect in an existing feature
Projects
None yet
3 participants