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

⚠️ Introduce v1alpha2 API #190

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Conversation

Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Jul 14, 2023

What this PR does / why we need it:
This PR introduces new API version v1alpha2.

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 #167

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 14, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Jul 14, 2023

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 14, 2023
@Fedosin Fedosin force-pushed the v1alpha2 branch 3 times, most recently from 52ecc4a to f023580 Compare July 18, 2023 16:10
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2023
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 10, 2023
@Fedosin Fedosin force-pushed the v1alpha2 branch 7 times, most recently from 05d51a2 to 78ad7d2 Compare August 11, 2023 09:51
@Fedosin
Copy link
Contributor Author

Fedosin commented Aug 11, 2023

/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 11, 2023
Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Thank you working on this, few suggestions inline:

api/v1alpha2/conditions_consts.go Outdated Show resolved Hide resolved
.golangci.yaml Show resolved Hide resolved
Comment on lines -260 to -261
if cSpec.Image != nil && cSpec.Image.Name != "" && cSpec.Image.Repository != "" {
c.Image = imageMetaToURL(cSpec.Image)
Copy link
Member

Choose a reason for hiding this comment

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

how are these changes related to new API version?

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is the reason for the new API version. Incompatible fields

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, @Danil-Grigorev is right. It's a result of the API change. In v1alpha1 we used ImageMeta object to provide a custom image for the provider. In v1alpha2 we decided that it's better to use the well-known /: image URL format, which is just a string.
After this change we don't have to convert ImageMeta -> ImageURL anymore, hence the change.

Copy link
Member

Choose a reason for hiding this comment

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

Should we add documentation for user migration from v1alpha1 to v1alpha2 in this case? Similar to https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/book/src/developer/providers/version-migration.md?plain=1

Copy link
Member

Choose a reason for hiding this comment

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

But migration doc in CAPI is formed when they release a minor release, and describe changes needed going from one minor release of CAPI to another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, you don't need to migrate anything manually. I implemented auto-conversion, which handles v1alpha1 -> v1alpha2 upgrade.
But I agree that it would be nice to have a doc describing the changes in the new version.

Copy link
Member

Choose a reason for hiding this comment

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

Auto-conversion looks good to me. If we face a situation when autogenerated conversion will not be enough, we could look towards documenting the process. Let’s address the doc describing changes closer to the next release.

@furkatgofurov7
Copy link
Member

Also forgot to mention, I would consider this as a breaking change and suggest re-titling the PR (with ⚠️) to signal it properly in the release notes

@Fedosin Fedosin changed the title ✨ Introduce v1alpha2 API ✨ ⚠️ Introduce v1alpha2 API Aug 15, 2023
@Fedosin Fedosin force-pushed the v1alpha2 branch 2 times, most recently from 2ddda48 to f5218d3 Compare August 15, 2023 09:37
Copy link
Member

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

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

/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 18, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d31cb9e373b23600b0d8f5d5832653313b9f28c9

@Danil-Grigorev
Copy link
Member

Thanks @Fedosin, looking good!

Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

/approve
/hold

  1. pending clarification on: ⚠️ Introduce v1alpha2 API #190 (comment). I would prefer taking care of this with the same PR if we decide to go down that path but no strong opinions since we have an autoconversion in place.
  2. in case we want to have more eyes on it

@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 23, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furkatgofurov7

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 23, 2023
@Fedosin
Copy link
Contributor Author

Fedosin commented Aug 23, 2023

@furkatgofurov7 I agree with you - some documentation is necessary here. I'll write it later today and update the PR.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2023
docs/README.md Outdated Show resolved Hide resolved

This document describes changes that were introduced in v1alpha2 API and how to update your templates to the new version.

## ImageMeta -> imageURL conversion
Copy link
Member

Choose a reason for hiding this comment

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

docs/migrating.md Outdated Show resolved Hide resolved
@furkatgofurov7
Copy link
Member

@Fedosin thanks a lot, this version looks great to me!

Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

/lgtm

Pending CI to pass (currently failing due to API rate limit and needs re-run after an hour)

@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

LGTM label has been added.

Git tree hash: 4dda14fbb1ad348a2cb26615b2dd40d4de0656b3

Copy link
Member

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

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

/lgtm

@Fedosin
Copy link
Contributor Author

Fedosin commented Aug 24, 2023

/retest

@Fedosin
Copy link
Contributor Author

Fedosin commented Aug 24, 2023

Thank you for review!

/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
@k8s-ci-robot k8s-ci-robot merged commit 7cf4782 into kubernetes-sigs:main Aug 24, 2023
@Fedosin Fedosin changed the title ✨ ⚠️ Introduce v1alpha2 API ⚠️ Introduce v1alpha2 API Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v1alpha2?
4 participants