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

Fix KubeadmControlPlane #21

Merged
merged 2 commits into from
Nov 8, 2023
Merged

Fix KubeadmControlPlane #21

merged 2 commits into from
Nov 8, 2023

Conversation

nprokopic
Copy link
Contributor

@nprokopic nprokopic commented Oct 21, 2023

What does this PR do?

This PR fixes control plane resources:

  • Add missing machine template to KubeadmControlPlane.

What is the effect of this change to users?

Control plane resources are templated correctly.

How does it look like?

Updated KubeadmControlPlane resource:

/spec  (KubeadmControlPlane/org-giantswarm/awesome)
  + two map entries added:
    machineTemplate:
      metadata:
        labels:
          app: cluster
          app.kubernetes.io/managed-by: Helm
          app.kubernetes.io/version: 0.1.0-dev
          application.giantswarm.io/team: turtles
          giantswarm.io/cluster: awesome
          giantswarm.io/organization: giantswarm
          giantswarm.io/service-priority: highest
          cluster.x-k8s.io/cluster-name: awesome
          cluster.x-k8s.io/watch-filter: capi
          helm.sh/chart: cluster-0.1.0-dev
          another-cluster-label: label-2
          some-cluster-label: label-1
      infrastructureRef:
        name: awesome-control-plane-f786309f
        apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
        kind: AWSMachineTemplate
    replicas: 3

Any background context you can provide?

giantswarm/roadmap#2742

@nprokopic nprokopic requested a review from a team as a code owner October 21, 2023 13:11
@github-actions
Copy link

() rendered manifest diff
/spec  (KubeadmControlPlane/org-giantswarm/awesome)
  + two map entries added:
    machineTemplate:
      metadata:
        labels:
          app: cluster
          app.kubernetes.io/managed-by: Helm
          app.kubernetes.io/version: 0.1.0-dev
          application.giantswarm.io/team: turtles
          giantswarm.io/cluster: awesome
          giantswarm.io/organization: giantswarm
          giantswarm.io/service-priority: highest
          cluster.x-k8s.io/cluster-name: awesome
          cluster.x-k8s.io/watch-filter: capi
          helm.sh/chart: cluster-0.1.0-dev
          another-cluster-label: label-2
          some-cluster-label: label-1
      infrastructureRef:
        name: awesome-control-plane-f786309f
        apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
        kind: AWSMachineTemplate
    replicas: 3

@@ -0,0 +1,8 @@
{{- define "cluster.internal.test.controlPlane.machineTemplate.spec" -}}
Copy link
Member

Choose a reason for hiding this comment

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

Is the test intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is just a "dummy" helper that is used in the CI.

@@ -51,4 +55,6 @@ spec:
{{- include "cluster.internal.controlPlane.kubeadm.postKubeadmCommands" $ | indent 4 }}
users:
{{- include "cluster.internal.kubeadm.users" $ | indent 4 }}
replicas: {{ .Values.global.controlPlane.replicas }}
version: v{{ trimPrefix "v" .Values.internal.kubernetesVersion }}
Copy link
Member

Choose a reason for hiding this comment

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

why do we have to trim the prefix v and then adding it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's in order to support .Values.internal.kubernetesVersion to both have and not have the v prefix, i.e. we don't care if the prefix is there, because we "clean up" the version from potential prefix and then we ensure it's there.

Took it from cluster-aws here https://github.com/giantswarm/cluster-aws/blob/acf1b9c712a51b364f5bece1481b3b3e3efdc263/helm/cluster-aws/templates/_control_plane.tpl#L247.

@nprokopic nprokopic merged commit ed29e38 into main Nov 8, 2023
@nprokopic nprokopic deleted the fix-resources branch November 8, 2023 13:31
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.

2 participants