Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

getGVR should only fail if no GroupVersionResource is found #4459

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

adduarte
Copy link

@adduarte adduarte commented Mar 7, 2023

The call to ServerPreferredResources could return an err != nil even though the resource list is not empty.

getGVR should not report a failure if the resource list does contain a matching GroupVersionResource, even if ServerPreferredResources returned err != nil.

What this PR does / why we need it

This patch modifies getGVR so it only return a error (failure) if it cannot find a matching GroupVersionResource.

Which issue(s) this PR fixes

Fixes #4460

Describe testing done for PR

Unit test created and executed.
ran deployed to vsphere 7 platform.

Release note

Fixes bug in clusterbootstrap webhook which incorrectly failed create validation if not all api service endpoints are running at the time of creation.

Additional information

Special notes for your reviewer

@adduarte adduarte requested review from a team as code owners March 7, 2023 21:33
@adduarte adduarte added the do-not-merge/wip Still work in progress label Mar 8, 2023
@adduarte adduarte force-pushed the topic/aduarte/ignore_discoverfailed_error branch from a7635f4 to ac3bc57 Compare March 8, 2023 08:56
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #4459 (243cbac) into main (5b41308) will decrease coverage by 0.91%.
The diff coverage is 0.00%.

❗ Current head 243cbac differs from pull request most recent head fc33b8f. Consider uploading reports for the commit fc33b8f to get more accurate results

@@            Coverage Diff             @@
##             main    #4459      +/-   ##
==========================================
- Coverage   49.66%   48.76%   -0.91%     
==========================================
  Files         452      482      +30     
  Lines       45152    47229    +2077     
==========================================
+ Hits        22425    23031     +606     
- Misses      20596    22014    +1418     
- Partials     2131     2184      +53     
Impacted Files Coverage Δ
addons/webhooks/clusterbootstrap_webhook.go 48.94% <0.00%> (+3.75%) ⬆️

... and 39 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@adduarte adduarte force-pushed the topic/aduarte/ignore_discoverfailed_error branch from 243cbac to ac3bc57 Compare March 9, 2023 10:25
@adduarte adduarte changed the title WIP: getGVR should only fail if no api resources are found getGVR should only fail if no api resources are found Mar 9, 2023
@adduarte adduarte force-pushed the topic/aduarte/ignore_discoverfailed_error branch from ac3bc57 to 0cc0817 Compare March 9, 2023 19:13
@adduarte adduarte force-pushed the topic/aduarte/ignore_discoverfailed_error branch 2 times, most recently from 2e44d41 to 9a52c4c Compare March 9, 2023 19:23
Copy link
Contributor

@shyaamsn shyaamsn left a comment

Choose a reason for hiding this comment

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

LGTM

@adduarte
Copy link
Author

adduarte commented Mar 9, 2023

/test install-vc7

@adduarte adduarte changed the title getGVR should only fail if no api resources are found getGVR should only fail if no GroupVersionResource is found Mar 9, 2023
@alfredthenarwhal
Copy link
Collaborator

@adduarte: /test install-vc7
Commit: 9a52c4c

Tests failed! Build no: 4398

@adduarte
Copy link
Author

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

@adduarte: /test install-vc7
Commit: 9a52c4c

Tests failed! Build no: 4404

@adduarte
Copy link
Author

/test install-vc7

@adduarte adduarte force-pushed the topic/aduarte/ignore_discoverfailed_error branch from 9a52c4c to 85afe58 Compare March 16, 2023 05:24
@adduarte
Copy link
Author

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

@adduarte: /test install-vc7
Commit: 85afe58

Tests failed! Build no: 4440

@adduarte
Copy link
Author

/test install-vc7

The call to ServerPreferredResources could return an error even
thoug the reource list is not empty.
This should not be a failure if the reource list is not empty.

This patch checks that if the error returned by ServerPreferredResoruces
is "ErrorGroupDiscoveryFailed", the getGVR will only return a error (failure)
if the resulting resource list is also empty.
@adduarte adduarte force-pushed the topic/aduarte/ignore_discoverfailed_error branch from 85afe58 to fc33b8f Compare March 16, 2023 08:12
@alfredthenarwhal
Copy link
Collaborator

@adduarte: /test install-vc7
Commit: 85afe58

Tests failed! Build no: 4443

@adduarte
Copy link
Author

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

@adduarte: /test install-vc7
Commit: fc33b8f

Tests passed! Build no: 4446

@adduarte
Copy link
Author

@adduarte adduarte added ok-to-merge PRs should be labelled with this before merging and removed do-not-merge/wip Still work in progress labels Mar 16, 2023
@adduarte adduarte merged commit d9bd44b into main Mar 17, 2023
@adduarte adduarte deleted the topic/aduarte/ignore_discoverfailed_error branch March 17, 2023 16:53
m1zzx2 pushed a commit that referenced this pull request Mar 27, 2023
The call to ServerPreferredResources could return an error even
thoug the reource list is not empty.
This should not be a failure if the reource list is not empty.

This patch checks that if the error returned by ServerPreferredResoruces
is "ErrorGroupDiscoveryFailed", the getGVR will only return a error (failure)
if the resulting resource list is also empty.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClusterBootstrap Webhook deny create request if any API service is not available
5 participants