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

feat(helm): use helm release namespace for all namespaced resources #15

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

BarthV
Copy link

@BarthV BarthV commented Jan 18, 2024

Explicit namespaced resources namespace value

Current Behavior

Current behavior is to rely on helm install capabilities to set resources namespace "just in time" during the setup phase.

helm install ejbca-cert-manager-issuer ejbca-issuer/ejbca-cert-manager-issuer --namespace ejbca-issuer-system --create-namespace

This is working pretty well

The problem

The problem is that the namespace is not applied if we're doing the same thing using helm template :

helm template ejbca-cert-manager-issuer deploy/charts/ejbca-cert-manager-issuer --namespace ejbca-issuer-system --create-namespace > no-ns.yaml
---
# Source: ejbca-cert-manager-issuer/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: ejbca-cert-manager-issuer 
< ! MISSING NAMESPACE HERE ! >
  labels:
    helm.sh/chart: ejbca-cert-manager-issuer-0.1.0
    app.kubernetes.io/name: ejbca-cert-manager-issuer
    app.kubernetes.io/instance: ejbca-cert-manager-issuer
    app.kubernetes.io/version: "v1.3.1"
    app.kubernetes.io/managed-by: Helm
spec:
  replicas: 1
  ...

This is caused by missing namespace value in helm templates for all namespaced resources (example for Deploy resource : https://github.com/Keyfactor/ejbca-cert-manager-issuer/blob/main/deploy/charts/ejbca-cert-manager-issuer/templates/deployment.yaml#L3-L6 )

This PR makes namespace value explicit in every resources that requires it, enabling a nice compatibility with helm template workflow.

Thanks !

@svenska-primekey
Copy link

@m8rmclaren Please review.

@fiddlermikey fiddlermikey requested a review from m8rmclaren March 12, 2024 15:57
@BarthV
Copy link
Author

BarthV commented Mar 29, 2024

@m8rmclaren gentle bump 👼

@m8rmclaren
Copy link
Contributor

@BarthV Excellent point - thank you for pointing this out. I will get this merged in the next release (hopefully today or early next week)

@m8rmclaren m8rmclaren changed the base branch from main to ci-53985 March 29, 2024 16:32
@m8rmclaren m8rmclaren merged commit 19cc46e into Keyfactor:ci-53985 Mar 29, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants