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

Work around upstream chart bugs/remove managed-by from templates (which are never managed) #53699

Closed
wants to merge 3 commits into from
Closed

Work around upstream chart bugs/remove managed-by from templates (which are never managed) #53699

wants to merge 3 commits into from

Conversation

bleggett
Copy link
Contributor

@bleggett bleggett commented Oct 29, 2024 •
edited
Loading

Please provide a description of this PR:
Also in kiali at least, fix this at the source so we don't have to mess with it again: kiali/helm-charts#294

Also fix the genscript (use yq, and use helm as the binary, helm2 hasn't shipped for years) and mention the existence of the genscript.

@bleggett
Work around upstream chart bugs/remove managed-by from templates (w…
d9cf10e 
…hich are never managed)

Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
@bleggett bleggett requested review from a team as code owners October 29, 2024 22:51
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 29, 2024
@bleggett bleggett mentioned this pull request Oct 29, 2024
Closed
2 tasks
howardjohn
howardjohn reviewed Oct 29, 2024
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why does this matter? Also this is all scripted in manifests/addons/gen.sh so will be reverted

If 4/4 charts we happen to sample -- which happen to include 2 of the most popular charts -- have this "bug", is it really a bug at that point...

@bleggett
Copy link
Contributor Author

Why does this matter? Also this is all scripted in manifests/addons/gen.sh so will be reverted

Ah, there is a gen script. I missed that. ty.

@bleggett
Copy link
Contributor Author

bleggett commented Oct 29, 2024 •
edited
Loading

Why does this matter?

Because it effectively marks things that were not installed by Helm as being managed by Helm, which will (potentially) confuse both Helm and other tools (and anyone or anything looking at the resources)

It's also just not needed.

If 4/4 charts we happen to sample -- which happen to include 2 of the most popular charts -- have this "bug", is it really a bug at that point

A popular bug, I will grant you. It only really matters when you use helm to template but not install (like we do), which is likely why it's not commonly hit.

@bleggett
Fixup genscript
f83a485 
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 29, 2024
@bleggett
Copy link
Contributor Author

@howardjohn Your call. If you don't want this we can close the PR.

@bleggett bleggett added the release-notes-none Indicates a PR that does not require release notes. label Oct 29, 2024
@bleggett
Don't bork the dashboards
4b4eec3 
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 29, 2024
@istio-testing
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 4, 2024
@bleggett
Copy link
Contributor Author

bleggett commented Nov 8, 2024

Closing this as 2/4 (soon 3/4) of the PRs opened in the upstream chart repos to fix this have merged, obviating this local workaround.

@bleggett bleggett closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn left review comments

Assignees
No one assigned
Labels
area/environments needs-rebase Indicates a PR needs to be rebased before being merged release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bleggett @istio-testing @howardjohn @istio-policy-bot