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

feat: make SubNamespace controller reconcile v2alpha1 version #111

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Nov 21, 2023

This PR changes the controllers to reconcile SubNamespace v1alpha2 version and makes the newer version the stored version in the CRD. The modified reconciler uses SSA to update SubNamespace using apply configurations generated in #106. I also plan to finalize migration to SSA for other resource types in use by controller.

Fixes #85

We should probably migrate the webhooks and kubectl-plugin to the newer version now. I can fix the hooks, but I have no experience with kubectl-plugins, so maybe you can assist in the migration?

@zoetrope
Copy link
Member

I will check to see if the plugin needs to be fixed.

@zoetrope
Copy link
Member

@erikgb
Apologies for the delayed response.

I've looked into the kubectl-plugins.
It appears that the plugin does not handle the Status in SubNamespace.
Additionally, since webhook will convert the resource, I see no need to change the implementation.
However, if there's a need for changes, please let me know, and I'll be happy to assist with the required updates.

@zoetrope
Copy link
Member

After this PR is merged, I will create a PR that will update the version of SubNamespace in the plugin.

@erikgb
Copy link
Contributor Author

erikgb commented Nov 29, 2023

@erikgb Apologies for the delayed response.

@zoetrope No worries! Accurate works well for us. We just want to support kstatus to get better user feedback when using tools like FluxCD or ArgoCD.

I've looked into the kubectl-plugins. It appears that the plugin does not handle the Status in SubNamespace. Additionally, since webhook will convert the resource, I see no need to change the implementation. However, if there's a need for changes, please let me know, and I'll be happy to assist with the required updates.

Thanks! If the plugin doesn't use status now, I think this change won't be problematic. At some point, we should probably deprecate the v1 API as a hint to users about migrating to the newer version. But I am hoping to fully migrate to SSA, which I hope can solve issues like #98.

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.

LGTM

@zoetrope zoetrope merged commit 38ea8a4 into cybozu-go:main Dec 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.

SubNamespace should be compatible with kstatus
2 participants