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

Migration of operator code to go/v4 schema #26

Merged
merged 6 commits into from
May 31, 2024

Conversation

ShivangGoswami
Copy link

@ShivangGoswami ShivangGoswami commented May 28, 2024

Issue: #24

The Required generated files and code changes are done in this PR to upgrade the Operator to go/v4
The Reconcile and other logic is carried forwarded from the old code and there is no code changes as such

These code also replaces kube-proxy,controller-gen,envtest,controller-tools to latest version, bumps go version of operator code and also use new schema definition of generated files

Testing:
The Operator was tested on openshift-crc with hyper foil v0.24.2 and is working as expected.

@ShivangGoswami
Copy link
Author

@barreiro @stalep plz review this PR

@lampajr lampajr self-requested a review May 28, 2024 18:26
@lampajr
Copy link
Member

lampajr commented May 28, 2024

Hi @ShivangGoswami, I am planning to review it tomorrow. I'll keep you posted 🙏

Copy link
Member

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

Hi @ShivangGoswami , here a first round: I had some issue building the operator locally and it looks like the issue was in the controller-gen version. Using a newer version seems doing the trick.

@ShivangGoswami ShivangGoswami requested a review from lampajr May 29, 2024 09:12
Copy link
Member

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

Hi again @ShivangGoswami, thanks for addressing all issues 🚀

I tested on both vanilla kubernetes and openshift cluster and it looks working.

I left just a minor comment on the README (sorry I did not notice this before), but apart from that it looks great to me!

@lampajr lampajr linked an issue May 29, 2024 that may be closed by this pull request
@ShivangGoswami ShivangGoswami requested a review from lampajr May 29, 2024 12:17
Copy link
Member

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

This one is solved for now. Will try to make readme more understandable in the coming days

No worries at all, I think that's already awesome 🚀

LGTM, thanks a lot again @ShivangGoswami !

@barreiro I leave the final decision to you, mainly to double check that I have not missed anything.

@lampajr lampajr mentioned this pull request May 29, 2024
Copy link
Contributor

@barreiro barreiro left a comment

Choose a reason for hiding this comment

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

Great work overall. I added a couple comments to ensure this won't break any workflows.

@ShivangGoswami ShivangGoswami requested a review from barreiro May 29, 2024 16:24
@barreiro barreiro merged commit 4d50296 into Hyperfoil:master May 31, 2024
1 check passed
@barreiro
Copy link
Contributor

@ShivangGoswami thank you for the contribution!

@barreiro barreiro mentioned this pull request May 31, 2024
@ShivangGoswami ShivangGoswami deleted the gov4schema-migration branch May 31, 2024 13:11
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.

Upgrade to go/v4
3 participants