⚠️ Remove use of apiutil.NewDynamicRESTMapper #2329
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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 |
k8s-ci-robot
added
the
approved
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to create a DynamicRESTMapper for Cluster %s/%s", cluster.Namespace, cluster.Name) | ||
| } | ||
| ret, err := client.New(restConfig, client.Options{Scheme: scheme, Mapper: mapper}) |
There was a problem hiding this comment.
This is causing the test failure because the test is trying to verify a non-error result from NewClusterClient, which currently wouldn't happen due to the test not providing a server for the client to connect to.
Do we have a need for a long lived remote client that would necessitate lazy discovery? I'm not sure we do.
| return nil, errors.Wrapf(err, "failed to create the controller-runtime DynamicRESTMapper") | ||
| } | ||
|
|
||
| c, err := client.New(config, client.Options{Scheme: Scheme, Mapper: mapper}) |
There was a problem hiding this comment.
@fabriziopandini is this proxy client a long lived client within clusterctl that would require the client to be setup and configured prior to the remote server listening? If not, then I think we can get away without lazy discovery and use the default controller-runtime client mapper.
There was a problem hiding this comment.
This client is used in many places, usually short-lived.
However, there are places like e.g. when installing cert-manager, where we create CRDs and immediately after we check for the CRDs to be available, and without the NewDynamicRESTMapper the client was not able to detect the new type (no matter if creating a new client or not)
There was a problem hiding this comment.
@fabriziopandini with the version of controller-runtime that we updated to use of the Dynamic REST Mapper is now the default. The behavior change introduced here is removal of the lazy discovery (which should only be needed if we absolutely need to be able to create a client before the server is available).
There was a problem hiding this comment.
The title of this PR is a little misleading as it is also changing the semantics of the code. In 2/3 places WithLazyDiscovery is being removed. I'm +1 on removing the NewDynamicRESTMapper in test/framework/management/kind/mgmt.go since the semantics don't change, but the other two look to intentionally be using WithLazyDiscovery.
I'd suggest either opening an issue to get feedback or rephrasing the title of the PR and using WithLazyDiscovery anyway.
detiber
changed the title
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
/retest |
|
/milestone v0.3.0-rc.1 |
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
added
size/M
detiber
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
This PR needs documentation about the API removal in https://master.cluster-api.sigs.k8s.io/developer/providers/v1alpha2-to-v1alpha3.html |
|
@vincepri added a statement to the doc for the remote client change. Other changes are net new for v1alpha3 anyway. |
k8s-ci-robot
added
the
needs-rebase
Since we are now using controller-runtime v0.5.0 we no longer need to override the default restmapper when initializing a controller-runtime client because the dynamic restmapper is now the default.
k8s-ci-robot
removed
the
needs-rebase
k8s-ci-robot
added
the
lgtm
What this PR does / why we need it:
Since we are now using controller-runtime v0.5.0 we no longer need
to override the default restmapper when initializing a controller-runtime
client because the dynamic restmapper is now the default.
This does change some of the semantics of how we are interacting with clients where we were previously setting
WithLazyDiscovery, but those changes should be limited just to the ability to get a client for a not-yet-ready remote server./assign @vincepri