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

fix(chart): explicitly set namespace based on release #435

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

valorl
Copy link
Contributor

@valorl valorl commented Nov 4, 2024

On helm install or upgrade, the --namespace flag is respected regardless.
However, when using this chart for static templating, the--namespace flag is ignored because the templates do not explicitly set .metadata.namespace to .Release.Namespace, making it cumbersome to customize the namespace.

@valorl valorl requested a review from a team as a code owner November 4, 2024 15:16
@valorl valorl force-pushed the fix/explicit-namespace branch from dbec4f0 to debef03 Compare November 4, 2024 15:17
@GMartinez-Sisti
Copy link
Member

Hi @valorl, thank you for the contribution.

Are you able to create a unit test to ensure that all resources have the required namespace label? Looks like wildcards on the template key is supported (docs).

@GMartinez-Sisti GMartinez-Sisti added the waiting-on-response Waiting for a response from the user label Nov 4, 2024
@valorl
Copy link
Contributor Author

valorl commented Nov 5, 2024

@GMartinez-Sisti hm I'm not really sure if I can test that with the current setup. This is purely an issue if you're helm templateing manifests. Anything that involves helm install (which, AFAICT, is how helm test works exclusively) will install to the provided namespace regardless of this change.

This would probably need to be tested separately e.g. by running the following:

helm template my-release -n my-namespace charts/atlantis --values charts/atlantis/test-values.yaml \
  | yq -e 'select(.metadata.namespace == "my-namespace")'

@valorl valorl force-pushed the fix/explicit-namespace branch from debef03 to 6746b6f Compare November 5, 2024 08:06
@GMartinez-Sisti
Copy link
Member

@GMartinez-Sisti hm I'm not really sure if I can test that with the current setup. This is purely an issue if you're helm templateing manifests. Anything that involves helm install (which, AFAICT, is how helm test works exclusively) will install to the provided namespace regardless of this change.

This would probably need to be tested separately e.g. by running the following:

helm template my-release -n my-namespace charts/atlantis --values charts/atlantis/test-values.yaml \
  | yq -e 'select(.metadata.namespace == "my-namespace")'

With unittest this is easy to achieve, all you need is a new file:

→ cat charts/atlantis/tests/misc_test.yaml
suite: test miscellaneous cases
templates:
  - "*.yaml"
chart:
  appVersion: test-appVersion
release:
  name: my-release
  namespace: my-namespace
tests:
  - it: ensure namespaces are specified in all resources
    asserts:
      - equal:
          path: metadata.namespace
          value: my-namespace

And then you can run make unit-test-run-atlantis and you actually see that there is an error:

→ make unit-test-run-atlantis
unittest        0.6.3   Unit test for helm chart in YAML with ease to keep your chart functional and robust.

### Chart [ atlantis ] ./charts/atlantis

 PASS  test configmap-gitconfig-init for gitconfig      charts/atlantis/tests/configmap-gitconfig-init_test.yaml
 FAIL  test miscellaneous cases charts/atlantis/tests/misc_test.yaml
        - ensure namespaces are specified in all resources

                - asserts[0] `equal` fail
                        Template:       atlantis/templates/configmap-config.yaml
                        Template:       atlantis/templates/configmap-gitconfig-init.yaml
                        Template:       atlantis/templates/configmap-init-config.yaml
                        Template:       atlantis/templates/configmap-repo-config.yaml
                        Template:       atlantis/templates/extra-manifests.yaml
                        Template:       atlantis/templates/podmonitor.yaml
                        Template:       atlantis/templates/role.yaml
                        Template:       atlantis/templates/rolebinding.yaml
                        Template:       atlantis/templates/secret-api.yaml
                        Template:       atlantis/templates/secret-aws.yaml
                        Template:       atlantis/templates/secret-basic-auth.yaml
                        Template:       atlantis/templates/secret-gitconfig.yaml
                        Template:       atlantis/templates/secret-netrc.yaml
                        Template:       atlantis/templates/secret-redis.yaml
                        Template:       atlantis/templates/secret-service-account.yaml
                        Template:       atlantis/templates/servicemonitor.yaml
                        Template:       atlantis/templates/webhook-ingress.yaml

 PASS  test pvc charts/atlantis/tests/pvc_test.yaml
 PASS  test secret-api for api secret   charts/atlantis/tests/secret-api_test.yaml
 PASS  test secret-aws for aws  charts/atlantis/tests/secret-aws_test.yaml
 PASS  test secret-basic-auth for git basic-auth secret charts/atlantis/tests/secret-basic-auth_test.yaml
 PASS  test secret-gitconfig for gitconfig      charts/atlantis/tests/secret-gitconfig_test.yaml
 PASS  test secret-netrc for netrc      charts/atlantis/tests/secret-netrc_test.yaml
 FAIL  test secret-service-account for serviceAccountSecrets    charts/atlantis/tests/secret-service-account_test.yaml
        - serviceAccountSecrets
                Error: template: atlantis/templates/secret-service-account.yaml:7:24: executing "atlantis/templates/secret-service-account.yaml" at <.Release.Namespace>: can't evaluate field Release in type interface {}

 PASS  test secret-webhook for git webhook secret       charts/atlantis/tests/secret-webhook_test.yaml
 PASS  test service     charts/atlantis/tests/service_test.yaml
 PASS  test statefulset charts/atlantis/tests/statefulset_test.yaml

Charts:      1 failed, 0 passed, 1 total
Test Suites: 2 failed, 10 passed, 12 total
Tests:       2 failed, 1 errored, 100 passed, 102 total
Snapshot:    0 passed, 0 total
Time:        650.929666ms

Error: plugin "unittest" exited with error
make: *** [Makefile:18: unit-test-run-atlantis] Error 1

This is because atlantis/templates/secret-service-account.yaml start with a range clause so you need to change the context to global: namespace: {{ .Release.Namespace }} to namespace: {{ $.Release.Namespace }}.

Please add the test and the fix and we should be good to merge!

@valorl valorl force-pushed the fix/explicit-namespace branch 2 times, most recently from 2f1e31f to 1c56c70 Compare November 5, 2024 12:26
@valorl
Copy link
Contributor Author

valorl commented Nov 5, 2024

Ah ok I misunderstood the tests then. I've fixed the range issue in the service account, but the actual misc test (also in your run) fails on any manifest that's optional. It seems that because of the *.yaml, all manifests are considered but they will be empty if their respective condition isn't true, which is then interpreted as an assert failure.

I've extended the test to set some dummy values to make sure no manifests are empty. It's passing now. Not sure if there's a better way to handle this.

On `helm install` or `upgrade`, the `--namespace` flag is respected
regardless. However, when using this chart for static templating, the
`--namespace` flag is ignored because the templates do not explicitly
set `.metadata.namespace` to `.Release.Namespace`, making it cumbersome
to customize the namespace.

Signed-off-by: valorl <[email protected]>
@valorl valorl force-pushed the fix/explicit-namespace branch from 1c56c70 to dd191d6 Compare November 5, 2024 12:31
Copy link
Member

@GMartinez-Sisti GMartinez-Sisti left a comment

Choose a reason for hiding this comment

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

Thanks!

@GMartinez-Sisti GMartinez-Sisti merged commit 329b842 into runatlantis:main Nov 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-response Waiting for a response from the user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants