-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Fix Helm Chart remove Capabilities.APIVersions for Kustomize to parse file #7829
Conversation
Welcome @stoupance! |
Hi @stoupance. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
/assign @ChiefAlexander |
/ok-to-test |
@rikatz for the release process of the helm chart do we need to have PR's bump the chart version? This looks good to me otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold for @rikatz
@ChiefAlexander @rikatz i think we decide when we have enough things to release or when is a critical fix/feature |
I disagree. I would rather we release fixes as they happen to get them out as quickly as possible. There should be no harm in bumping a bugfix version and turning that around. Even minor fixes. |
i agree with you @ChiefAlexander I was just explaining how was before, not sure if we want to continue having that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is added here to prevent incorrect configuration from causing exceptions.
And this is a Helm chart, I don’t think it is reasonable to delete this condition.
/hold
It is not used in the other CRD's for this. Given that the option |
i agree with @ChiefAlexander or I'm missing something that I cannot see/understand |
|
The problem I am considering here is the following situation:
If we assume that the user knows what they are doing, then this condition can be removed. (Feel free to remove the hold and merge it. ) |
Personally, servicemonitor is just too complicated. Unless the docs can be
updated to teach and support service monitors, this may not end up being an
improvement.
Thanks,
; Long
…On Mon, 25 Oct, 2021, 9:14 AM Jintao Zhang, ***@***.***> wrote:
The problem I am considering here is the following situation:
➜ ingress-nginx git:(stoupance/main) ✗ helm install ingress -n 7829-ingress --set controller.metrics.enabled=true --set controller.metrics.serviceMonitor.enabled=true charts/ingress-nginx
Error: unable to build kubernetes objects from release manifest: unable to recognize "": no matches for kind "ServiceMonitor" in version "monitoring.coreos.com/v1"
If we assume that the user knows what they are doing, then this condition
can be removed. (Feel free to remove the hold and merge it. )
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#7829 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGZVWXSID536IB2YDEF44TUITHAXANCNFSM5GMCNBHQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
@tao12345666333 Well, this error is thrown by your cluster because I assume you don't have the proper (Prometheus) CRDs installed? Which is, in fact, an expected and "normal" error. It goes the same way with any operator, if it's not installed, then you cannot use its CRDs. Nothing can protect a user from this, this is the default behaviour. There is actually 3 conditions in the template file
If you want to keep condition 1, I can understand this, then it must be also added in this other template file because both are using the same CRDs (keep in mind doing this will then prevent any usage of this Helm chart with Indeed, condition 1 cannot be present in one file, and not in another template file which both are tied to the same CRDs. This is not logical. And this condition is actually causing an issue when used by Kustomize. That being said, the third condition (
@longwuyuan I don't understand your point. The In this case, when using So, it's not an improvement, but a bug fix while some users (me for example) cannot use this Helm chart because of this "double checks" when one is totally irrelevant. |
Sure. The situation I mentioned above is that when the user did not install the relevant CRD, the option was turned on by mistake. So adding this condition is equivalent to a stricter restriction. Strict restrictions have not caused any bugs, on the contrary, it reduces the probability of users seeing errors and can bring a better user experience. I think the reason here is that kustomize does not fully support all the capabilities of Helm (and this is not the native usage of Helm). ref: kubernetes-sigs/kustomize#3815 (comment)
|
How is this going? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@longwuyuan @tao12345666333 friendly ping to follow up here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/kind bug
/triage accepted
/priority important-longterm
@stoupance , any chance you can create a issue and link it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/lgtm Let's move forward with this /hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afirth, cpanato, rikatz, stoupance The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
As fixed in pull request kubernetes#7829 for the ServiceMonitor resource, this is also needed for the PrometheusRule. When upgrading the ingress-nginx chart in our environment (via Pulumi) from a really old version to the latest (4.2.0) we noticed it wanted to delete the PrometheusRule resource. This PR should fix that.
As fixed in pull request #7829 for the ServiceMonitor resource, this is also needed for the PrometheusRule. When upgrading the ingress-nginx chart in our environment (via Pulumi) from a really old version to the latest (4.2.0) we noticed it wanted to delete the PrometheusRule resource. This PR should fix that.
What this PR does / why we need it:
Types of changes
Which issue/s this PR fixes
This PR fixes the issue initially described here. The Helm chart provided could not customized by
Kustomize
to build the YAML section forserviceMonitor
. Indeed this Helm template file has the following test as first line.So Kustomize with Helm (
kubectl kustomize . --enable-helm=true
) build locally the YAML from this template and could not pass the first test( .Capabilities.APIVersions.Has "monitoring.coreos.com/v1" )
for obvious reasons. Test failed and then did not output, as expected, the final YAML section forserviceMonitor
.This test irrelevant and is not present in other template files, for example like the one regarding Prometheus to build the
prometheusRule
.This issue is not presents with vanilla Helm and only appears when
Kustomize
is used on top ofHelm
How Has This Been Tested?
controller-servicemonitor.yaml
file and pushed it into my forked repokubectl kustomize . --enable-helm=true
)serviceMonitor
as expectedChecklist: