NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

fix(helm): ensure coder can be deployed in a non-default namespace#16579

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ThomasK33 merged 1 commit intomainfrom02-14-fix_helm_ensure_coder_can_be_deployed_in_a_non-default_namespace

Feb 18, 2025

Merged

fix(helm): ensure coder can be deployed in a non-default namespace#16579

ThomasK33 merged 1 commit intomainfrom02-14-fix_helm_ensure_coder_can_be_deployed_in_a_non-default_namespace

Feb 18, 2025

Conversation

Copy link

Member

ThomasK33 commentedFeb 14, 2025•
edited
Loading

Added namespace to all resources in the helm chart and added tests to ensure that coder can be deployed in non-default namespaces, as specified via the namespace flag in the helm command.

Ways to verify this:

current state:

$ helm template my-coder coder -n coder --version 2.19.0 --repo https://helm.coder.com/v2| yq'.metadata.namespace'null---null---null---null---null

fixed state when checking out this PR:

$ helm template my-coder ./helm/coder -n coder --set coder.image.tag=latest| yq'.metadata.namespace'coder---coder---coder---coder---coder

Change-Id: Ib66d4be9bcc4984dfe15709362e1fe0dcd3e847f
Signed-off-by: Thomas Kosiewskitk@coder.com

fix(helm): ensure coder can be deployed in a non-default namespace

b7f2d12

Change-Id: Ib66d4be9bcc4984dfe15709362e1fe0dcd3e847fSigned-off-by: Thomas Kosiewski <tk@coder.com>

Copy link

MemberAuthor

ThomasK33 commentedFeb 14, 2025

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

github-actionsbot assignedThomasK33

Feb 14, 2025

ThomasK33 marked this pull request as ready for review

February 14, 2025 15:27

ThomasK33 requested review fromjohnstcn andmafredri

February 14, 2025 15:28

johnstcn approved these changes

Feb 14, 2025

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, but do weneed to do this for every single test case? Would simply testing with the default values suffice?

Copy link

MemberAuthor

ThomasK33 commentedFeb 14, 2025

LGTM, but do weneed to do this for every single test case? Would simply testing with the default values suffice?

I don't think testing the default values would be enough here. For example, the ingress is not enabled by default; thus, if it were to miss the namespace, we'd not be able to catch it.
The same applies to all the other objects that get conditionally templated.

Copy link

abasu0713 commentedFeb 14, 2025

I would like to add the issue affects the following CRDs for a failed deployment using Terraform helm provider:

RoleBinding
Role
Deployment
Service

I haven't used Ingress, since my setup is Cloudflare Tunnels -> Coder Service (ClusterIP) which avoids the need for Ingress, Load balancers or NodePorts with Egress only routes within my network.
Hope this helps.
@ThomasK33 Thanks for sending the PR. You rock! Looking forward to getting this merged.