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.

Sign up for GitHub

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

Jump to bottom

fix(helm): explicitly include namespace in manifests #1606

Closed
wants to merge 1 commit into from
Closed

fix(helm): explicitly include namespace in manifests #1606

wants to merge 1 commit into from

Conversation

valorl
Copy link

@valorl valorl commented Sep 4, 2024 •
edited
Loading

Currently, most of the manifests omit .metadata.namespace.

This works fine with helm install / helm upgrade where all the manifests without a namespace will be installed to --namespace anyway.

However, helm template will produce the manifests as-is (with no namespace), regardless of --namespace, forcing users to add the namespace manually or use more tooling.

This change adds the namespace to all manifests.

Fixes #1607

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@valorl
fix(helm): explicitly include namespace in manifests
f2e596a 
Currently, most of the manifests omit `.metadata.namespace`.

This works fine with `helm install` / `helm upgrade`
where all the manifests without a namespace will be installed to `--namespace`
anyway.

However, `helm template` will produce the manifests as-is (with
no namespace), regardless of `--namespace`, forcing users to add the namespace manually
or use more tooling.

This change adds the namespace to all manifests.
@CLAassistant
Copy link

CLAassistant commented Sep 4, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 5, 2024

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@neopointer
Copy link

@clayton-cornell I'm just tagging a random contributor here. What is this PR missing to get merged?

@clayton-cornell clayton-cornell requested a review from a team November 13, 2024 20:11
@clayton-cornell
Copy link
Contributor

clayton-cornell commented Nov 13, 2024 •
edited
Loading

@neopointer It looks like this one got missed in the shuffle. Lets see if we can get a developer/maintainer review happening here.

@grafana/grafana-alloy-maintainers can someone from the team take a look at this?

@mattdurham
Copy link
Collaborator

This looks reasonable to me and the namespace defaults to release.namespace so should be backwards compatible for how it was meant to be used.

mattdurham
mattdurham approved these changes Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

LGTM

@mattdurham
Copy link
Collaborator

@petewall any thoughts on if this causes any issue?

@mattdurham
Copy link
Collaborator

This PR #2044 seems to be the most active and achieves the same result?

mattdurham
mattdurham requested changes Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

Looking to see if we can consolidate PRs

@valorl
Copy link
Author

valorl commented Dec 5, 2024 •
edited
Loading

Closing in favor of #2044, it's more complete and achieves the same.

@valorl valorl closed this Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattdurham mattdurham mattdurham requested changes

Assignees
No one assigned
Labels
needs-attention
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helm chart does not work well with "helm template"
5 participants
@valorl @CLAassistant @neopointer @clayton-cornell @mattdurham