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

🌱 Conversion of references should use a CR client #5160

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

vincepri
Copy link
Member

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:

This changeset introduces some deprecations for functions that are still
creating the metadata client through a restconfig each time the function
is called. The controller runtime client supports retrieving metadata
only objects; this change deprecates the old functions and creates new
ones that use a CR client.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

/milestone v0.4

@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Aug 26, 2021
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 26, 2021
@vincepri
Copy link
Member Author

/assign @sbueringer @fabriziopandini

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 26, 2021
@vincepri vincepri force-pushed the update-contract-ref branch from 2f42881 to 622289f Compare August 26, 2021 14:56
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 26, 2021
This changeset introduces some deprecations for functions that are still
creating the metadata client through a restconfig each time the function
is called. The controller runtime client supports retrieving metadata
only objects; this change deprecates the old functions and creates new
ones that use a CR client.

Signed-off-by: Vince Prignano <[email protected]>
@vincepri vincepri force-pushed the update-contract-ref branch from 622289f to 6c13858 Compare August 26, 2021 15:02
@fabriziopandini
Copy link
Member

This is a great cleanup, thanks!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 26, 2021
Copy link
Member Author

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit dbc0120 into kubernetes-sigs:master Aug 26, 2021
@ykakarap
Copy link
Contributor

ykakarap commented Aug 26, 2021

I see it is merged but the comment in this line is using the wrong function name: https://github.com/kubernetes-sigs/cluster-api/blob/master/util/conversion/conversion.go#L57. It should be UpdateReferenceAPIContract.

@ykakarap
Copy link
Contributor

I expected the linter to pick this up. Looks like it does not.

@vincepri
Copy link
Member Author

@ykakarap 🤔 I thought it'd have picked it up as well, we might want to see why

@sbueringer
Copy link
Member

I'll fix it in the somewhat related #5143 PR

@sbueringer
Copy link
Member

sbueringer commented Aug 26, 2021

@vincepri @ykakarap I think it's because godot is per default only enabled on declarations: https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml#L259-L267

Unfortunately, we can't enable it for now everywhere because it panics somewhere in clusterctl: tetafro/godot#20 (comment)

Sooner or later I'd like to fix godot so that we can enable it everywhere.

@ykakarap
Copy link
Contributor

I will open an issue to fix the godot lints everywhere in our codebase, following that we can enable godot everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants