Allows the release namespace to be overridden

[KhizerJaan]

Problem:

• Vault can only be deployed in Release namespace.

Root Cause:

• namespace is hardcoded to {{ .Release.Namespace }}

Solution:

• Templatize namespaec in _helpers.tpl

[hashicorp-cla]

All committers have signed the CLA.

[tomhjp]

Hi @KhizerJaan, thanks for your interest and PR! Please could you provide some more context about why this is useful?

[KhizerJaan]

Hello @tomhjp , If Vault Helm Chart is being used as dependency in another helm (umbrella) chart, it is deployed to umbrella chart release namespace . This PR would enabled to override namespace for Vault Helm Chart.

[KhizerJaan]

@tomhjp kindly review and merge the pull request at your convenience. I have umbrella chart use case and am blocked by it . default namespace is set to Release.Namespace , so it won't effect current behavior and would add an extra feature.
By allowing users to choose the deployment namespace, Helm charts become more versatile and adaptable to different environments and deployment strategies. Users can deploy the chart in the namespace that aligns with their specific requirements or organizational practices .

[tomhjp]

Thanks for the explanation, that makes sense. This seems like a nice improvement to merge then with a few additions. Please could we add some bats unit tests for each resource, a default empty value in the Values file, an entry in the schema, and a short changelog entry under unreleased improvements? Happy to help out with any of those, just let me know if you need some pointers or assistance.

[KhizerJaan]

Sure. Highly appreciate the feedback and suggestions @tomhjp . I will incorporate the suggested changes, and submit a revised version that addresses the feedback provided.

[KhizerJaan]

@tomhjp I have incorporated the suggestions provided.
Kindly review the updated PR at your convenience. I believe that it is now complete and addresses all the necessary improvements. However, I welcome any further feedback or suggestions you may have to enhance the quality of the contribution.

[KhizerJaan]

@tomhjp Could you please consider merging this PR? Alternatively, I would greatly appreciate any feedback you might have.

[tomhjp]

Thanks for the changes so far - just a few more small suggestions and then I think this is ready to merge.

[tomhjp]

I ran the CI and a few unit test failures came up FYI. Judging from the test names, looks like they should be trivial fixes.

not ok 166 injector/certs-secret: namespace is set in 131ms
not ok 168 injector/role: namespace is set in 130ms
not ok 170 injector/rolebinding: namespace is set in 135ms
not ok 174 injector/MutatingWebhookConfiguration: namespace is set in 138ms

[KhizerJaan]

@tomhjp Thank you for suggestions . highly appreciate it . I've updated regards to your suggestions . PTAL

[KhizerJaan]

@tomhjp PTAL

[KhizerJaan]

@tomhjp can this be merged please.

[tomhjp]

Sorry, the changelog changed in main since my last review - please could you rebase/merge in main to make sure the changes stay in the unreleased section when this is merged? Otherwise LGTM

[KhizerJaan]

@tomhjp branch synced . PTAL

[KhizerJaan]

@tomhjp can you please merge this PR? I've synced with main . or it would be great if you have any feedback.

[tomhjp]

I've just run the tests again to prepare for merging, but it looks like the same ones from before are still failing. Do you need some help getting going with running them locally?

not ok 166 injector/certs-secret: namespace is set
not ok 168 injector/role: namespace is set
not ok 170 injector/rolebinding: namespace is set
not ok 174 injector/MutatingWebhookConfiguration: namespace is set

[KhizerJaan]

@tomhjp previously results were escaped like this \"bar\" and it was failing locally so i removed escaping characters . now I've reverted back to escaping . can you please test now?

[tomhjp]

Looks like that's done the trick, thanks. The other way to fix it would be to add -r to the yq command, like yq -r '.webhooks[0].clientConfig.service.namespace', but what you've got now LGTM.