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

Use namespace from kube context when namespace not supplied #69

Merged
merged 7 commits into from
Dec 22, 2023

Conversation

marksmithson
Copy link
Contributor

If the namespace attribute is not supplied to the rule helm is called with --namespace=default.

As a result the rule cannot be used to install the chart into the current namespace for the kubernetes context.

This MR prevents the --namespace argument being passed to helm when it is not specified.

This change could affect users who are relying on the chart being installed in the default namespace where their kube context specifies an alternative namespace to default. This is unlikely, however can be mitigated by users specifying namespace="default" when calling the rule

@pmoncadaisla
Copy link
Contributor

Hi @marksmithson , thanks for your contribution! 🎉

I have some concerns:

  1. I'm not sure if relying on an external config not managed by Bazel is a deterministic approach.
  2. It is true that it is not backward-compatible, what about adding extra input attribute to the rule? (I.E. use_kubecontext_namespace)

Tell us what you think about this. Thanks!

@marksmithson
Copy link
Contributor Author

  1. I'm not sure if relying on an external config not managed by Bazel is a deterministic approach.

The generated chart and perhaps the installation script are what should be deterministic, not the effect of running these scripts in an external environment (outside of bazel). This change does not affect that - it is just that the NAMESPACE is "" and not default

It is true that it is not backward-compatible, what about adding extra input attribute to the rule? (I.E. use_kubecontext_namespace)

How about we just set the default for the namespace attribute to "default"? Users can then set this to "" to use whatever default namespace is in their kubectl context? Would need a doc update as well.

( ideally the default would be "", but using "default" is a reasonable compromise for backward campatibility. )

Note that some clusters may have deleted the default namespace, so it may not be a great "default" :)

@pmoncadaisla
Copy link
Contributor

I like "" as default better than "default", but since it breaks the backward compatibility we should set it as you suggest. We can also add a ⚠️ deprecation notice and change the behavior in the future, and make the "" default.

@marksmithson
Copy link
Contributor Author

Done.

Fixed up some attribute names in docs whilst I was there.

Formatted version look like this:

image

Copy link
Contributor

@pmoncadaisla pmoncadaisla left a comment

Choose a reason for hiding this comment

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

LGTM!

@danigar , feel free to merge!

@danigar danigar merged commit d9660c9 into masmovil:master Dec 22, 2023
1 check 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.

4 participants