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

Remove scheme parameter of CreateK8sCR and ORANO2IMSReconciler #88

Merged

Conversation

jhernand
Copy link
Collaborator

It isn't necessary to explicitly pass the scheme paratemer to the CreateK8sCR function because it can obtain it calling the Scheme() function of the client.

@openshift-ci openshift-ci bot requested review from danielerez and irinamihai April 11, 2024 19:01
Copy link

openshift-ci bot commented Apr 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jhernand. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024
It isn't necessary to explicitly pass the scheme paratemer to the
`CreateK8sCR` function because it can obtain it calling the `Scheme()`
function of the client.

Signed-off-by: Juan Hernandez <[email protected]>
@jhernand jhernand force-pushed the remove_scheme_parameter_of_createk8scr branch from 3759020 to e8ae589 Compare April 11, 2024 19:06
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024
@irinamihai irinamihai changed the title Remove scheme parameter of CreateK8sCR Remove scheme parameter of CreateK8sCR and ORANO2IMSReconciler Apr 11, 2024
@irinamihai
Copy link
Collaborator

irinamihai commented Apr 11, 2024

Hmm, I do wonder why kubebuilder adds *runtime.Scheme by default to the Reconciler structure if it's not actually needed 🤔
The controller-runtime example does not use it, so I think we should be ok.

Kindly add more info on this (as a comment here or readme) if you have it, it would be greatly appreciated.

@irinamihai
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2024
@jhernand jhernand merged commit 707c065 into openshift-kni:main Apr 12, 2024
7 of 8 checks passed
@jhernand jhernand deleted the remove_scheme_parameter_of_createk8scr branch April 12, 2024 08:00
@jhernand
Copy link
Collaborator Author

Kindly add more info on this (as a comment here or readme) if you have it, it would be greatly appreciated.

I don't really have more information, just wanted to reduce the number of parameters of the CreateK8sCR function and noticed that the scheme can be obtained from the client. I will take a look at what kubebuilder does and update if I find some better explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants