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

Stop serving v1alpha2 version of the ClusterGroup CRD #4812

Merged

Conversation

antoninbas
Copy link
Contributor

The API was scheduled for deletion a year ago.
My proposal is to stop serving the API in the next release (v1.12), and remove it completely in the release after that (v1.13). If someone upgrades to Antrea v1.12 and gets errors, they can easily edit the CRD to re-enable serving for the older version (and then convert all their existing resources).

@antoninbas antoninbas added api-review Categorizes an issue or PR as actively needing an API review. area/api Issues or PRs related to an API. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API. action/release-note Indicates a PR that should be included in release notes. labels Apr 5, 2023
tnqn
tnqn previously approved these changes Apr 6, 2023
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

Dyanngg
Dyanngg previously approved these changes Apr 6, 2023
Copy link
Contributor

@Dyanngg Dyanngg left a comment

Choose a reason for hiding this comment

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

LGTM

The API was scheduled for deletion a year ago.
My proposal is to stop serving the API in the next release (v1.12), and
remove it completely in the release after that (v1.13). If someone
upgrades to Antrea v1.12 and gets errors, they can easily edit the CRD
to re-enable serving for the older version (and then convert all their
existing resources).

Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
@antoninbas antoninbas dismissed stale reviews from Dyanngg and tnqn via 7bf0aa7 April 6, 2023 16:55
@antoninbas antoninbas force-pushed the stop-serving-v1alpha2/ClusterGroup branch from d69bd1e to 7bf0aa7 Compare April 6, 2023 16:55
Copy link
Contributor

@Dyanngg Dyanngg left a comment

Choose a reason for hiding this comment

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

E2E test seems to be failing though. Let me take a look as well

@antoninbas
Copy link
Contributor Author

@Dyanngg Yes, I need to remove the e2e tests that use the API

@Dyanngg
Copy link
Contributor

Dyanngg commented Apr 6, 2023

E2E testcase testACNPAppliedToDenyXBtoCGWithYA still builds a v1alpha2 version of ClusterGroup and test that it can be used. We need to change the API version to v1a3

Signed-off-by: Antonin Bas <abas@vmware.com>
@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas antoninbas requested a review from Dyanngg April 6, 2023 20:29
}
return nil, fmt.Errorf("error occurred in creating/updating ClusterGroup %s", cg.Name)
}

// CreateOrUpdateV1Alpha3CG is a convenience function for idempotent setup of crd/v1alpha3 ClusterGroups
func (data *TestData) CreateOrUpdateV1Alpha3CG(cg *crdv1alpha3.ClusterGroup) (*crdv1alpha3.ClusterGroup, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we just rename it to CreateOrUpdateClusterGroup then (same for the delete and builder functions), otherwise when API is graduated to v1beta this will be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me. If v1beta1 is not backwards-compatible with v1alpha3, then we can always introduce the version suffix back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'd rather not change it in this PR. I see that we have the same issue for the Group CRD, even though we ever only had one version for that one. We should probably handle them together as a separate PR, or when we introduce v1beta1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'll take note on this one

@antoninbas antoninbas requested review from tnqn and Dyanngg April 7, 2023 18:47
@antoninbas antoninbas merged commit 4f6e9aa into antrea-io:main Apr 7, 2023
@antoninbas antoninbas deleted the stop-serving-v1alpha2/ClusterGroup branch April 7, 2023 21:13
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
* Stop serving v1alpha2 version of the ClusterGroup CRD

The API was scheduled for deletion a year ago.
My proposal is to stop serving the API in the next release (v1.12), and
remove it completely in the release after that (v1.13). If someone
upgrades to Antrea v1.12 and gets errors, they can easily edit the CRD
to re-enable serving for the older version (and then convert all their
existing resources).

* Remove API references from e2e tests

Signed-off-by: Antonin Bas <abas@vmware.com>
GraysonWu added a commit to GraysonWu/antrea that referenced this pull request Jul 19, 2023
…-io#4812)"

This reverts commit 4f6e9aa.

Signed-off-by: graysonwu <wgrayson@vmware.com>
GraysonWu added a commit to GraysonWu/antrea that referenced this pull request Jul 19, 2023
…-io#4812)"

This reverts commit 4f6e9aa.

Signed-off-by: graysonwu <wgrayson@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. api-review Categorizes an issue or PR as actively needing an API review. area/api Issues or PRs related to an API. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants