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

chore: change v2alpha1 SubNamespace status to kstatus-compatible subresource #96

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Oct 14, 2023

A new and currently unserved API version was added with #101. This PR makes status in the new version a subresource compatible with kstatus.

I have also added conversion code with unit tests, so you can get an idea of how I plan to move forward after this is (hopefully) merged. The next steps will probably be:

  1. Add conversion webhook using the conversion logic established in this PR.
  2. Make v2alpha1 served, migrate controller to reconcile v2alpha1 and make v2alpha1 the storage version.

I would prefer using SSA when updating status from controller in the latter step. Please let me know what you think! This is probably best done by generating apply-configurations in a separate pre-PR.

Relates to #85

NOTE: The proposed fuzz conversion tests added in this PR are highly inspired by the equivalent test setup in https://github.com/kubernetes-sigs/cluster-api. Example: https://github.com/kubernetes-sigs/cluster-api/blob/main/api/v1alpha4/conversion_test.go

@erikgb
Copy link
Contributor Author

erikgb commented Oct 14, 2023

If you want, I can split this PR:

  1. Establish the new API version as a copy of v1
  2. Make the new version compatible with kstatus

@erikgb
Copy link
Contributor Author

erikgb commented Oct 15, 2023

I have now created a dedicated PR for (just) introducing the new API version: #101

@zoetrope zoetrope self-requested a review October 19, 2023 02:08
@erikgb erikgb force-pushed the kstatus branch 3 times, most recently from 0256ce4 to ca986bc Compare October 27, 2023 18:27
@erikgb erikgb changed the title WIP: feat: make SubNamespace compatible with kstatus chore: make v2alpha1/SubNamespace API kstatus compatible Oct 27, 2023
@erikgb erikgb marked this pull request as ready for review October 27, 2023 18:45
@erikgb erikgb changed the title chore: make v2alpha1/SubNamespace API kstatus compatible chore: convert v2alpha1 SubNamespace status to kstatus-compatible subresource Oct 27, 2023
@erikgb
Copy link
Contributor Author

erikgb commented Oct 27, 2023

@zoetrope @ymmt2005 I think this PR is ready for a round of review now. Please let me know what you think!

@erikgb erikgb changed the title chore: convert v2alpha1 SubNamespace status to kstatus-compatible subresource chore: change v2alpha1 SubNamespace status to kstatus-compatible subresource Oct 27, 2023
Copy link
Member

@zoetrope zoetrope 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 for the excellent pull request!
I believe the PR looks great.

I wasn't familiar with the "kstatus" library before, but it looks interesting.
In situations like this, I often use apimachinery's Condition.

@erikgb
Copy link
Contributor Author

erikgb commented Nov 1, 2023

Thank you for the excellent pull request! I believe the PR looks great.

I wasn't familiar with the "kstatus" library before, but it looks interesting. In situations like this, I often use apimachinery's Condition.

Thanks for the nice feedback, @zoetrope! We are also heavy users of apimachinery's Condition in our operators not yet migrated to SSA. I think the utilities in this package are mostly useful with CSA. But we will get to that in the follow-up PRs. 😉

@erikgb
Copy link
Contributor Author

erikgb commented Nov 6, 2023

Is there anything more needed to get this merged?

@zoetrope zoetrope merged commit 07b61c1 into cybozu-go:main Nov 8, 2023
8 checks passed
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