Automatically fill in .Capabilities in Helm Charts #2155
Conversation
|
PR is now waiting for a maintainer to run the acceptance tests. |
yann-soubeyrand
force-pushed
the
automatically-fill-capabilities-in-helm-charts
branch
from
4e1b595
to
fada353
Compare
|
PR is now waiting for a maintainer to run the acceptance tests. |
yann-soubeyrand
force-pushed
the
automatically-fill-capabilities-in-helm-charts
branch
from
fada353
to
00049f3
Compare
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/2952219436 |
There was a problem hiding this comment.
This looks great; thanks for the PR @yann-soubeyrand! Just one minor suggestion, and then LGTM if the tests pass.
| // Preserve backward compatibility | ||
| if len(c.opts.APIVersions) > 0 { | ||
| installAction.APIVersions = c.opts.APIVersions | ||
| } |
There was a problem hiding this comment.
I'd check this first, and skip the discovery client calls to the apiserver if true.
There was a problem hiding this comment.
Hello @lblackstone, thanks for the review 😉
I followed your suggestion and modified the code, but now I’m wondering if we really want to disable Kubernetes version discovery when APIVersions are provided. What do you think?
Regarding the tests, it seems they haven’t run, do I have to fix something on my side?
There was a problem hiding this comment.
I posted a message in Slack (https://pulumi-community.slack.com/archives/CRFURDVQB/p1661613221358179) to get some directions on how to develop on this provider, maybe you have the answers to my questions?
There was a problem hiding this comment.
I followed your suggestion and modified the code, but now I’m wondering if we really want to disable Kubernetes version discovery when APIVersions are provided. What do you think?
Ah, I didn't notice that the DefaultCapabilities was setting the kube version before, and defaulted to v1.20. Maybe we should set that explicitly here? The problem is that the cluster may not be available yet when this code is running (i.e., if it's a preview in a stack where the cluster hasn't been created yet), so we need a fallback value set.
Regarding the tests, it seems they haven’t run, do I have to fix something on my side?
For external contributors, a maintainer has to run the tests with the /run-acceptance-tests comment, and then the bot will respond with a link to the test run that you can check: #2155 (comment)
It looks like everything passed, so this should be good once we handle the default k8s version.
There was a problem hiding this comment.
I followed your suggestion and modified the code, but now I’m wondering if we really want to disable Kubernetes version discovery when APIVersions are provided. What do you think?
Ah, I didn't notice that the
DefaultCapabilitieswas setting the kube version before, and defaulted to v1.20. Maybe we should set that explicitly here? The problem is that the cluster may not be available yet when this code is running (i.e., if it's a preview in a stack where the cluster hasn't been created yet), so we need a fallback value set.
The default KubeVersion seems to still be set here https://github.com/helm/helm/blob/dbc6d8e20fe1d58d50e6ed30f09a04a77e4c68db/pkg/action/install.go#L216.
My concern was about the fact that people having APIVersions manually set won’t benefit from the discovery of the KubeVersion. But it’s not a regression. We could also add the ability to manually set the KubeVersion (as for the APIVersions), but that’d be a fairly different PR 😉
Regarding the tests, it seems they haven’t run, do I have to fix something on my side?
For external contributors, a maintainer has to run the tests with the
/run-acceptance-testscomment, and then the bot will respond with a link to the test run that you can check: #2155 (comment)It looks like everything passed, so this should be good once we handle the default k8s version.
Ha OK, I had a notification with this link https://github.com/pulumi/pulumi-kubernetes/actions/runs/2939221543 which made me think the tests didn’t run, I didn’t see https://github.com/pulumi/pulumi-kubernetes/actions/runs/2952219436, so that’s OK 😉
Closes pulumi#196 Closes pulumi#753 Signed-off-by: Yann Soubeyrand <yann.soubeyrand@gmx.fr>
yann-soubeyrand
force-pushed
the
automatically-fill-capabilities-in-helm-charts
branch
from
00049f3
to
40e4b7a
Compare
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-kubernetes/actions/runs/2965080398 |
|
Thank you @yann-soubeyrand for doing this PR as it fixes the issue I had in #2141 (I've tested locally to check it does everything my issue needed it to do) |
|
Thanks @lblackstone for the prompt review 😉 |
yann-soubeyrand
deleted the
automatically-fill-capabilities-in-helm-charts
branch
|
@yann-soubeyrand I see only apiVersions in the Chart's options but I would have expected also kubeVersions to solve the issue you had in the first place and that I'm currently having: #2249 |
|
@LucasBrazi06 to solve this issue, I added the ability to automatically fill in .Capabilities (which contains Kubernetes version) in Helm Charts to this provider. Therefore, the (existing) apiVersions attribute should not be needed nor the (non-existing) kubeVersion attribute. |
Proposed changes
This PR tries to add automatic filling of Helm capabilities (Kubernetes version and API versions) in Chart resource. I’ve successfully built and tested this PR with a simple chart, but I’m not familiar with development on this repository (on Pulumi providers in general), so I may need some help to bring this PR to proper releasable state.
Related issues (optional)
Fix #196
Fix #2141
Fix #753