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

Add namespace to generated manifests #193

Conversation

mdostal-hci
Copy link
Contributor

What this PR does / why we need it:

It explicitely adds namespace to generated manifests (e.g. by helm template), not to rely on automated Helm inclusion. See #192 for more detail.

Which issue this PR fixes

fixes #192

Special notes for your reviewer:

Checklist

  • PR is based off the current tip of the next branch and targets next, not main
  • Title of the PR and commit headers start with chart name (e.g. [kong])

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2020

CLA assistant check
All committers have signed the CLA.

@hbagdi hbagdi changed the base branch from main to next September 2, 2020 16:13
@rainest
Copy link
Contributor

rainest commented Sep 17, 2020

Hey, more places to tag @eskibars !

This hits a vein similar to the end of Kong/kubernetes-ingress-controller#844 (review), where we have most of our more complex tooling in this Helm chart, but have some users that don't want all of Helm's overhead.

Chart modifications to support Helm use via helm template (which only renders manifests from templates, and does not handle lifecycle management) may be an imperfect middle ground: helm template allows for workflows that are similar to what Kustomize provides, while leveraging our existing templates for more advanced functionality. We have have encountered users who ran into issues with this approach in the past, as using helm template to render templates and then apply them outside Helm's lifecycle management isn't really supported as a first-class/expected use case by Helm itself (I may be wrong there--checking with their devs to see how much that is or isn't the case), so we may need to proceed with caution and/or better document gotchas if we go the helm template route.

I've historically been reticent to add documentation or template functionality targeted at helm template use as such, though in this particular case the change isn't something that would cause obvious problems when using helm install and doesn't require much maintenance going forward, so I'd be pretty okay merging this if we increase endorsement of helm template use while acknowledging some of the rough edges.

@mdostal-hci though this doesn't relate to the PR content itself, you're a demonstrably good person to ask know that we know you do use the chart that way: what other stumbling blocks have you encountered, if any? In particular, how do you handle migration jobs? Do you find that you need to do much beyond provide --is-upgrade and extract them from the larger body of rendered templates?

Side note: approving but not merging yet--mostly just keeping this open because I'm interested in the broader discussion between helm template users, myself, and members of our product strategy team. We don't have an imminent chart release; this should go in for the next one, however.

Copy link
Member

@hbagdi hbagdi left a comment

Choose a reason for hiding this comment

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

As Travis said, we don't have a lot of feedback from users who do the helm template workflow. We are happy to incorporate such improvements and even document it further if that helps the community.

@rainest
Copy link
Contributor

rainest commented Sep 25, 2020

Meh, go figure, other PR created conflicts. Merged manually in 700d287 to avoid anything similar going forward. We'll keep the stuff for product management in mind, as we have other open tickets for that elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a reason why some templates does not contain namespace?
4 participants