-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 ClusterResourceSet not getting Secret/Configmap TypeMeta #4129
🐛Fix ClusterResourceSet not getting Secret/Configmap TypeMeta #4129
Conversation
bcc2137
to
3341e0f
Compare
@sedefsavas Can you open a PR to the main branch first? |
3341e0f
to
306801e
Compare
306801e
to
1c85ffb
Compare
/retest |
/hold |
1c85ffb
to
982bd07
Compare
/hold cancel |
Moved PR to the main branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of comments.
Also are we sure that the tests will catch the error above? I tried to get the tests to fail but couldn't. Maybe I'm missing something?
982bd07
to
f1cdf7b
Compare
/test pull-cluster-api-test-main |
@sedefsavas Is the test failure related? It seems to be failing on CRS tests |
err := r.Client.Scheme().Convert(resourceInterface, raw, nil) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err := r.Client.Scheme().Convert(resourceInterface, raw, nil) | |
if err != nil { | |
if err := r.Client.Scheme().Convert(resourceInterface, raw, nil); err != nil { |
Passing locally and controllers test failed there. /test pull-cluster-api-test-main |
It passed now, retesting. |
This is looking good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[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 |
/milestone v0.4.0 @sedefsavas Can you open a cherry-pick PR to release-0.3 once this merges? |
What this PR does / why we need it:
After using live client for ConfigMap and Secrets, when doing get on the resources,
TypeMeta
is returned nil.Using a different conversion fills
apiVersion
andkind
fields.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 #4115