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.

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

✨ Include Kubernetes version in helm template #243

Merged
merged 3 commits into from
Aug 30, 2024

Conversation

chess-knight
Copy link
Member

What this PR does / why we need it:
Helm charts can use Capabilities.KubeVersion.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes SovereignCloudStack/cluster-stacks#143

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squash commits
  • include documentation
  • add unit tests

Signed-off-by: Roman Hros <roman.hros@dnation.cloud>
@batistein batistein self-requested a review August 14, 2024 10:00
@batistein
Copy link
Contributor

@chess-knight While reviewing this PR, I noticed that a new flag was added without a strong reason behind it.
There are reasons not to include these helm capabilities by design, as this would require testing the helm logic. Typically, it is best practice to avoid a lot of templating logic that goes beyond the Kubernetes version. Since the cluster stacks are mainly used for a very strong, deeply integrated lifecycle, it is better not to include these helm capabilities.

Of course, I understand that this may seem convenient when using a third-party dependency.
But if we want to add the build in capabilities then we should do it for all of them. Right now, for example, APIVersion is missing and would also be used in the same context.

@chess-knight
Copy link
Member Author

chess-knight commented Aug 14, 2024

@chess-knight While reviewing this PR, I noticed that a new flag was added without a strong reason behind it. There are reasons not to include these helm capabilities by design, as this would require testing the helm logic. Typically, it is best practice to avoid a lot of templating logic that goes beyond the Kubernetes version. Since the cluster stacks are mainly used for a very strong, deeply integrated lifecycle, it is better not to include these helm capabilities.

The reason is the cilium helm chart, which doesn't work for k8s < 1.30 when installation is done by helm template, please see the related issue SovereignCloudStack/cluster-stacks#143

Of course, I understand that this may seem convenient when using a third-party dependency. But if we want to add the build in capabilities then we should do it for all of them. Right now, for example, APIVersion is missing and would also be used in the same context.

Yes, this is true. helm template supports also --api-versions flag. But IMO adding this for allowing usage of Capabilities.APIVersions is much harder compared to this PR, where the k8s version can be extracted directly from metadata. We can add it later if there is demand.
EDIT: Maybe it is not so hard, just use kubectl api-versions and kubectl api-resources commands before the helm template. Or, we can probably reuse the helm install/upgrade logic and include helm go functions in the CSO go code, so we don't need to parse outputs. We should think about this enhancement for sure. I suggest creating a separate issue from this and merging this PR, so we can release a new CSO version and fix Cilium issues until the R7 milestone.

@jschoone
Copy link
Contributor

Hi @chess-knight and @janiskemper since this is approved, can it be merged?

@chess-knight
Copy link
Member Author

Hi @chess-knight and @janiskemper since this is approved, can it be merged?

IMHO yes

@janiskemper
Copy link
Member

yes

@chess-knight
Copy link
Member Author

It is strange. One unit test failed, but I merged only docs changes. Before that change, all tests passed.

@janiskemper
Copy link
Member

I was successful in re-running ;)

@chess-knight chess-knight merged commit 3dc37fb into main Aug 30, 2024
6 checks passed
@chess-knight chess-knight deleted the feat/cluster_addon_kube_version branch August 30, 2024 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openstack-scs-1-29-v1 / openstack-scs-1-28-v2 not deployable (cilium issues)
4 participants