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

Allows to disable helm validation #18297

Closed
wants to merge 1 commit into from
Closed

Allows to disable helm validation #18297

wants to merge 1 commit into from

Conversation

farodin91
Copy link
Contributor

@farodin91 farodin91 commented Dec 17, 2021 •
edited
Loading

Fixes #16552

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #issue-number

Allows to disable helm validation

@farodin91 farodin91 requested a review from a team as a code owner December 17, 2021 12:10
@farodin91 farodin91 requested review from a team, gandro and christarazi December 17, 2021 12:10
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 17, 2021
@farodin91
Allows to disable helm validation
5f4e9a8 
Fixes cilium#16552

Signed-off-by: Jan Jansen <jan.jansen@gdata.de>
@farodin91 farodin91 force-pushed the allows-to-disable-helm-validation branch from 7b9bdca to 5f4e9a8 Compare December 17, 2021 15:01
@farodin91 farodin91 requested a review from a team as a code owner December 17, 2021 15:01
qmonnet
qmonnet requested changes Dec 17, 2021
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks! The change looks good overall. It should be noted that it disables the whole Helm validation, when there was just one check that seemed to cause an issue in the original bug report, but I think it's OK.

One thing I would like is to keep a bit more context for the change. Could you please explain why this change is required, and why helm template doesn't work in your commit description? It's not fun to go and search for the PRs when git blaming a change to find the initial motivation. I'm also wondering whether we should extend the description of the new option to tell users why it might be desirable to disable the checks.

install/kubernetes/cilium/values.yaml
@@ -1798,3 +1798,6 @@ cgroup:
# -- Configure whether to enable auto detect of terminating state for endpoints
# in order to support graceful termination.
enableK8sTerminatingEndpoint: true

# -- Allows to disable the helm validation checks
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# -- Allows to disable the helm validation checks
# -- Allows to disable the Helm validation checks

install/kubernetes/cilium/values.yaml
@@ -1798,3 +1798,6 @@ cgroup:
# -- Configure whether to enable auto detect of terminating state for endpoints
# in order to support graceful termination.
enableK8sTerminatingEndpoint: true

# -- Allows to disable the helm validation checks
enableHelmValidation: true
Copy link
Member

Choose a reason for hiding this comment

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

Please preserve the line break at the end of the line.

@qmonnet qmonnet added the release-note/misc This PR makes changes that have no direct user impact. label Dec 17, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 17, 2021
@qmonnet qmonnet added needs-backport/1.10 release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Dec 17, 2021
gandro
gandro reviewed Dec 20, 2021
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks! Please also run make -C install/kubernetes to update the Helm Chart README.

@gandro
Copy link
Member

gandro commented Dec 20, 2021

While I'm not opposed to be able to opt-out from the validation, I would like to point out that the referenced issue is better solved by specifying the supported api-versions manually rather than disabling validation: #16552 (comment)

christarazi
christarazi reviewed Dec 20, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi 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.

Thanks for the PR.

My opinion is that it seems like a clunky approach to workaround a problem when installing Helm charts. Ideally, we shouldn't need to provide an opt-out if the solution to the root issue is what @gandro suggested. @farodin91 Has that worked for you or are there more use cases for opting out of Helm validation?

@farodin91
Copy link
Contributor Author

@gandro I'm currently playing around with ArgoCD. It seems the issue is solved with Argo CD 2.2. I'm okay with closing this PR. It could be helpful to document how to run helm template correctly.

@gandro
Copy link
Member

gandro commented Dec 21, 2021

@gandro I'm currently playing around with ArgoCD. It seems the issue is solved with Argo CD 2.2. I'm okay with closing this PR. It could be helpful to document how to run helm template correctly.

I see. Looking at argoproj/argo-cd#7291 indeed seems that Argo CD has remaining bugs around --api-versions depending on how it is used, which might justify adding this as a workaround for now.

@farodin91
Copy link
Contributor Author

@gandro I'm currently playing around with ArgoCD. It seems the issue is solved with Argo CD 2.2. I'm okay with closing this PR. It could be helpful to document how to run helm template correctly.

I see. Looking at argoproj/argo-cd#7291 indeed seems that Argo CD has remaining bugs around --api-versions depending on how it is used, which might justify adding this as a workaround for now.

Validation issue of the servicemonitor is solved in argo 2.2. I would add a workaround only, if there another issue.

I've you want that cilium wants this workaround, i'll update the PR to fix all comments.

@gandro
Copy link
Member

gandro commented Dec 21, 2021

Validation issue of the servicemonitor is solved in argo 2.2. I would add a workaround only, if there another issue.

I'm not aware of any other issues related to validation at the moment. So yes, let's close this then. We can always re-open if we discover another issue.

@farodin91 Thanks a ton for looking into this and doing the work. Even if this will not be merged, your contribution and input was most welcome!

@gandro gandro closed this Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro left review comments

@christarazi christarazi christarazi left review comments

@qmonnet qmonnet qmonnet requested changes

Assignees

@gandro gandro

@christarazi christarazi

@qmonnet qmonnet

Labels
release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helm template with serviceMonitor enabled fails
5 participants
@farodin91 @gandro @christarazi @qmonnet @joestringer