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

Fix: mapping failed. #660

Merged

Conversation

xuezhaojun
Copy link
Contributor

@xuezhaojun xuezhaojun commented Sep 13, 2023

Fixes: https://issues.redhat.com/browse/ACM-7479

Root cause: Start with this PR, the behavior of NewDynamicRESTMapper changed.

Run this restMapper.KindFor(&schema.GroupVersionResource{Resource:"deployment"}}), the results:

Before it returns a GroupVersionKind:

schema.GroupVersionKind{ Group: "apps", Kind: "Deployment", Version: "v1"}

But now, it returns the error:

no matches for kind "deployment" in version ""

The root cause is in controller-runtime v0.14.6, it reloads by this function: https://github.com/kubernetes-sigs/controller-runtime/blob/cd65cb25d314f40a329a688f4714fe3282589e97/pkg/client/apiutil/dynamicrestmapper.go#L94 which caches all groups and resource from discoveryClient.

But after this PR, it reloads by ServerResourcesForGroupVersion which makes version a required value, and that against our API design of ManagedClusterView.

@zhiweiyin318
Copy link
Contributor

/lgtm
/approve
/hold for verification on the env.

Signed-off-by: xuezhaojun <zxue@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@xuezhaojun
Copy link
Contributor Author

/unhold

@zhiweiyin318
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 13, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 13, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xuezhaojun, zhiweiyin318

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

@openshift-merge-robot openshift-merge-robot merged commit 2d8d035 into stolostron:main Sep 13, 2023
12 checks passed
@xuezhaojun xuezhaojun deleted the fix-restmapping-fail branch September 13, 2023 14:15
nirs added a commit to nirs/ramen that referenced this pull request Sep 14, 2023
The fix[1] for bug[2] seems to break ManagedClusterView for custom
resources (like VolumeReplicationGroup). Ramen works with previous
commit[3].

[1] stolostron/multicloud-operators-foundation#660
[2] https://issues.redhat.com/browse/ACM-7479
[3] stolostron/multicloud-operators-foundation@031ee0f

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants