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

Add KubevirtClusterTemplate resource to support ClusterClass feature #251

Merged
merged 3 commits into from
Aug 28, 2023
Merged

Add KubevirtClusterTemplate resource to support ClusterClass feature #251

merged 3 commits into from
Aug 28, 2023

Conversation

aamoyel
Copy link
Contributor

@aamoyel aamoyel commented Jul 25, 2023

Fix: #250

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 25, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2023
aamoyel
aamoyel commented Jul 25, 2023
View reviewed changes
pkg/workloadcluster/workloadcluster.go Outdated
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 made a change on this line cause when you use ClusterClass and KubevirtClusterTemplate, the final KubevirtCluster has prefix like a replicaset (eg: cluster-xpejq). The real kubeconfig was created with the "Cluster" resource name by the CAPI Control Plane controller.

@agradouski
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 25, 2023
@coveralls
Copy link

coveralls commented Jul 25, 2023 •
edited

Pull Request Test Coverage Report for Build 5985539652

  • 4 of 67 (5.97%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.6%) to 49.492%
Changes Missing Coverage Covered Lines Changed/Added Lines %
api/v1alpha1/zz_generated.deepcopy.go 0 63 0.0%
Totals Coverage Status
Change from base Build 5940382564: -1.6%
Covered Lines: 926
Relevant Lines: 1871
💛 - Coveralls

@agradouski
Copy link
Contributor

@davidvossel note, this is an alpha feature of CAPI, any objections to support it in CAPK at this time?

@agradouski
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2023
aamoyel
aamoyel commented Aug 2, 2023
View reviewed changes
pkg/loadbalancer/loadbalancer.go Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like the Workload Cluster, we need to use the cluster name for LB metadata.name cause KubevirtCluster add a prefix on kubevirtCluster ressource when ClusterClass and KubevirtClusterTemplate is used.

davidvossel
davidvossel reviewed Aug 17, 2023
View reviewed changes
Copy link
Contributor

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

hey, I haven't looked deeply at the ClusterClass feature, but this api seems fine.

The only part I question is why we need a validating webhook to ensure this template remains immutable after creation. Can't we express immutability using the crd CEL validation?

api/v1alpha1/kubevirtclustertemplate_types.go Outdated
Comment on lines 42 to 43

Spec KubevirtClusterTemplateSpec `json:"spec,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add something like the validation below to ensure immutability without needing a webhook?
// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="spec is immutable"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello David, thx for your review :) .
I didn't know it was possible to enforce CRD Immutability with CEL Rules, thanks for the advice. I will update my PR to match your specification.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 18, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 18, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 18, 2023
@nunnatsa
Copy link
Contributor

/hold

github action CI didn't run yet.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2023
@davidvossel
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023
@nunnatsa
Copy link
Contributor

/hold

The ci was not run yet

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023
davidvossel
davidvossel approved these changes Aug 24, 2023
View reviewed changes
Copy link
Contributor

@davidvossel davidvossel left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 24, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aamoyel, davidvossel

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2023
@davidvossel
Copy link
Contributor

github action CI didn't run yet.

oh? I see that the e2e integration test passed. What else are we waiting on?

@nunnatsa
Copy link
Contributor

nunnatsa commented Aug 24, 2023 •
edited

github action CI didn't run yet.

oh? I see that the e2e integration test passed. What else are we waiting on?

Unit tests, linters, check-gen, etc.

Admins should see the "Approve and Test" button. Clicking on it will enable these tests

@nunnatsa
Copy link
Contributor

github action CI didn't run yet.

oh? I see that the e2e integration test passed. What else are we waiting on?

Unit tests, linters, check-gen, etc.

Admins should see the "Approve and Test" button. Clicking on it will enable these tests

@agradouski @cchengleo ^^^

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 26, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 26, 2023
@aamoyel
Copy link
Contributor Author

aamoyel commented Aug 26, 2023

Can you replay tests please @agradouski @davidvossel ?

@nunnatsa
Copy link
Contributor

/unhold

Thanks @aamoyel !

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2023
@nunnatsa
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2023
@k8s-ci-robot k8s-ci-robot merged commit b5ebf7b into kubernetes-sigs:main Aug 28, 2023
10 of 11 checks passed
@aamoyel aamoyel deleted the cluster-class branch August 28, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidvossel davidvossel davidvossel approved these changes

@nunnatsa nunnatsa Awaiting requested review from nunnatsa

@qinqon qinqon Awaiting requested review from qinqon

Assignees

@davidvossel davidvossel

@nunnatsa nunnatsa

@agradouski agradouski

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support of CAPI ClusterClass feature
6 participants
@aamoyel @agradouski @coveralls @nunnatsa @davidvossel @k8s-ci-robot