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

[WIP] Make helm charts respect namespace parameter #517

Closed
wants to merge 2 commits into from

Conversation

lblackstone
Copy link
Member

Fixes #217

@lblackstone lblackstone requested a review from hausdorff April 3, 2019 15:14
@lblackstone
Copy link
Member Author

Hmm, looks like I'm not handling the namespacing properly for CRDs. Will fix that and retry.

Copy link
Contributor

@hausdorff hausdorff left a comment

Choose a reason for hiding this comment

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

I'm not completely sure that I fully understand the implications of this change. Because this change is fairly risky, I think it's worth asking a few "dumb" questions just to make sure we're all on the same page.

  • Does auto-adding and then stripping the namespace affect how the resource ID is allocated? If so, is it possible that this will trigger replaces for existing stacks that use namespace or set namespace from the transformations API?
  • Does this changing of the resource definitions match what the customers expect? i.e., if namespace is automatically populated in the diff view, will it confuse users?

You can also imagine packing the namespace as a separate field into the properties, which already contains both the old inputs and the new inputs. Philosophically this is closer to what kubectl --namespace and helm --namespace do -- the --namespace flag does not change the resource definition, but instead, changes the namespace component of the POST request route.

It's hard to say whether that approach is better, since then you'd have to incorporate that logic into (e.g.) Diff. For me the main question I don't know is: which of these things does the user expect, and which of these things has a smaller chance of breaking in visible ways?

@lblackstone lblackstone force-pushed the lblackstone/helm-namespace branch from 6ace6e0 to 90e5e71 Compare April 4, 2019 02:58
@lblackstone lblackstone force-pushed the lblackstone/helm-namespace branch from 90e5e71 to a0a215e Compare April 4, 2019 16:00
@lblackstone lblackstone changed the title Make helm charts respect namespace parameter [WIP] Make helm charts respect namespace parameter Apr 4, 2019
@lblackstone
Copy link
Member Author

This approach is not working out for CRs.

The problem is that the required CRD is often unregistered at the Check stage, so the provider is unable to determine if the resource is namespaceable. This can only be determined after the CRD is created. Unfortunately, this doesn't work cleanly with the preview and resource naming logic, which doesn't have access to an official source of truth.

This leads to two conclusions:

  1. We should pass the namespace in the provider opts as @hausdorff suggested rather than putting it in the object spec since this affects previews.
  2. We should decouple the resource names from the namespace since we can't reliably determine that information prior to creation.

I'm going to rework this and revert #506 in the meantime since it relies on the same faulty assumptions.

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.

helm: apply namespace default transformation
2 participants