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

[CAFV-130] Upgrade API to v1beta2 #374

Merged
merged 12 commits into from
Feb 22, 2023
Merged

[CAFV-130] Upgrade API to v1beta2 #374

merged 12 commits into from
Feb 22, 2023

Conversation

abaruni
Copy link
Contributor

@abaruni abaruni commented Jan 27, 2023 •
edited
Loading

Description

upgrade api version to v1beta2 and complete any required automatic and manual conversions from v1alpha4 -> v1beta2 & v1beta1 -> v1beta2

➜  ~ kube get VCDCluster.v1alpha4.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDCluster.v1beta1.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDCluster.v1beta2.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDMachine.v1alpha4.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDMachine.v1beta1.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDMachine.v1beta2.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDMachineTemplate.v1alpha4.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDMachineTemplate.v1beta1.infrastructure.cluster.x-k8s.io
No resources found in default namespace.
➜  ~ kube get VCDMachineTemplate.v1beta2.infrastructure.cluster.x-k8s.io
No resources found in default namespace.

Checklist

  • tested locally
  • updated any relevant dependencies
  • updated any relevant documentation or examples

API Changes

Are there API changes?

  • Yes
  • No

If yes, please fill in the below

  1. Updated conversions?
    • Yes
    • No
    • N/A
  2. Updated CRDs?
    • Yes
    • No
    • N/A
  3. Updated infrastructure-components.yaml?
    • Yes
    • No
    • N/A
  4. Updated ./examples/capi-quickstart.yaml?
    • Yes
    • No
    • N/A
  5. Updated necessary files under ./infrastructure-vcd/v1.0.0/?
    • Yes
    • No
    • N/A

Issue

If applicable, please reference the relevant issue

Fixes #

This change is Reviewable

Anirudh9794
Anirudh9794 reviewed Jan 31, 2023
View reviewed changes
api/v1beta1/vcdcluster_conversion.go Outdated
// ConvertTo converts this VCDCluster to the Hub version (v1beta2).
func (src *VCDCluster) ConvertTo(dstRaw conversion.Hub) error {
dst := dstRaw.(*v1beta2.VCDCluster)
return Convert_v1beta1_VCDCluster_To_v1beta2_VCDCluster(src, dst, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to have the logic to restore fields using utilconversion.UnmarshalData(src, restored)

api/v1beta1/vcdcluster_conversion.go Outdated
// ConvertFrom converts from the Hub version (v1beta2) to this version (v1beta1).
func (dst *VCDCluster) ConvertFrom(srcRaw conversion.Hub) error {
src := srcRaw.(*v1beta2.VCDCluster)
return Convert_v1beta2_VCDCluster_To_v1beta1_VCDCluster(src, dst, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

All the ConvertFrom() functions should store the dst object in an annotation in src using utilconversion.MarshalData(src, dst)

sahithi
sahithi approved these changes Jan 31, 2023
View reviewed changes
Copy link
Collaborator

@sahithi sahithi left a comment

Choose a reason for hiding this comment

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

:lgtm: after addressing the comment and testing one successful cluster creation with VCDCluster objects in v1beta2 format

Please create another Jira task to test the below scenarios

Case1: CAPI yaml with core capi objects in v1beta1 and our objects in v1beta1
Case2: CAPI yaml with core capi objects in v1beta1 and our objects in v1beta2
Case3: CAPI yaml with core capi objects in v1alpha4 and our objects in v1alpha4

Reviewed 40 of 43 files at r1, 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @abaruni and @arunmk)

config/crd/kustomization.yaml line 2 at r2 (raw file):

commonLabels:
  cluster.x-k8s.io/v1beta2: v1beta2

This is incorrect. We have not yet upgraded core capi to v1beta2. These labels dictate the API contract between core capi and capvcd. It says - any given version of core capi must use which version of capvcd.

It should be something like
cluster.x-k8s.io/v1beta1: v1beta1_v1beta2

This needs to be tested out thoroughly.
Case1: CAPI yaml with core capi objects in v1beta1 and our objects in v1beta1
Case2: CAPI yaml with core capi objects in v1beta1 and our objects in v1beta2
Please create another task for testing if you want to submit this PR.

abaruni
abaruni commented Feb 21, 2023
View reviewed changes
Copy link
Contributor Author

@abaruni abaruni left a comment

Choose a reason for hiding this comment

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

Reviewable status: 40 of 53 files reviewed, 3 unresolved discussions (waiting on @Anirudh9794, @arunmk, and @sahithi)

api/v1beta1/vcdcluster_conversion.go line 11 at r2 (raw file):

Previously, Anirudh9794 wrote…

This needs to have the logic to restore fields using utilconversion.UnmarshalData(src, restored)

Done.

api/v1beta1/vcdcluster_conversion.go line 17 at r2 (raw file):

Previously, Anirudh9794 wrote…

All the ConvertFrom() functions should store the dst object in an annotation in src using utilconversion.MarshalData(src, dst)

Done.

config/crd/kustomization.yaml line 2 at r2 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…

This is incorrect. We have not yet upgraded core capi to v1beta2. These labels dictate the API contract between core capi and capvcd. It says - any given version of core capi must use which version of capvcd.

It should be something like
cluster.x-k8s.io/v1beta1: v1beta1_v1beta2

This needs to be tested out thoroughly.
Case1: CAPI yaml with core capi objects in v1beta1 and our objects in v1beta1
Case2: CAPI yaml with core capi objects in v1beta1 and our objects in v1beta2
Please create another task for testing if you want to submit this PR.

Done.

@abaruni abaruni force-pushed the CAFV-130/api-upgrade branch from e693766 to fb947f8 Compare February 21, 2023 22:16
Anirudh9794
Anirudh9794 approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@Anirudh9794 Anirudh9794 left a comment

Choose a reason for hiding this comment

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

:lgtm: with a small comment

Reviewed 32 of 43 files at r1, 7 of 8 files at r2, 13 of 13 files at r3, 20 of 20 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @abaruni, @arunmk, and @sahithi)

api/v1beta1/vcdmachinetemplate_conversion.go line 27 at r3 (raw file):

func (dst *VCDMachineTemplate) ConvertFrom(srcRaw conversion.Hub) error {
	src := srcRaw.(*v1beta2.VCDMachineTemplate)
	return Convert_v1beta2_VCDMachineTemplate_To_v1beta1_VCDMachineTemplate(src, dst, nil)

utilconversion.MarshalData(src, dst) is missing from this function

@abaruni abaruni merged commit 6efd28f into vmware:main Feb 22, 2023
@abaruni abaruni deleted the CAFV-130/api-upgrade branch February 22, 2023 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anirudh9794 Anirudh9794 Anirudh9794 approved these changes

@sahithi sahithi sahithi approved these changes

@arunmk arunmk Awaiting requested review from arunmk

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abaruni @sahithi @Anirudh9794 @vmwclabot