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

fix issue with querying for GVR in capabilities discovery package #4272

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

yharish991
Copy link
Member

What this PR does / why we need it

When querying for existence of GVR without version, capabilities discovery package returns false even though the resource exists. This PR is for fixing that issue.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

Verified that the units tests are passing and wrote a small sample program to test GVR query functionality

Release note

Fixed query GVR functionality when a version is not provided in capabilities discovery package.

Additional information

Special notes for your reviewer

@yharish991 yharish991 requested review from a team as code owners January 11, 2023 23:15
@yharish991 yharish991 changed the title fix issue with querying for GVR in capabilities client fix issue with querying for GVR in capabilities discovery package Jan 11, 2023
Copy link
Contributor

@codegold79 codegold79 left a comment

Choose a reason for hiding this comment

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

Thanks for finding this bug and fixing it so quickly, @yharish991.

@yharish991 yharish991 force-pushed the fixCapabilityQueryGVR branch from 3e9a054 to 15ae8bc Compare January 11, 2023 23:39
@yharish991 yharish991 requested a review from a team as a code owner January 11, 2023 23:39
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4272/20230111235059/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #4272 (87d3b83) into main (f39341f) will decrease coverage by 0.90%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #4272      +/-   ##
==========================================
- Coverage   49.40%   48.50%   -0.91%     
==========================================
  Files         451      481      +30     
  Lines       44755    46878    +2123     
==========================================
+ Hits        22111    22737     +626     
- Misses      20560    22004    +1444     
- Partials     2084     2137      +53     
Impacted Files Coverage Δ
capabilities/client/pkg/discovery/cluster_gvr.go 83.60% <92.30%> (-0.84%) ⬇️
addons/controllers/machine_controller.go 65.65% <0.00%> (-3.04%) ⬇️
...ons/controllers/packageinstallstatus_controller.go 79.15% <0.00%> (ø)
.../cli/plugin/cluster/get_machinehealthcheck_node.go 9.30% <0.00%> (ø)
cmd/cli/plugin/isolated-cluster/test/main.go 0.00% <0.00%> (ø)
cmd/cli/plugin/cluster/scale.go 17.85% <0.00%> (ø)
...olated-cluster/imagepushop/publishimagesfromtar.go 73.17% <0.00%> (ø)
cmd/cli/plugin/cluster/create.go 42.22% <0.00%> (ø)
.../isolated-cluster/imgpkginterface/client_imgpkg.go 0.00% <0.00%> (ø)
... and 25 more

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

@yharish991 yharish991 force-pushed the fixCapabilityQueryGVR branch from 15ae8bc to 850e5f2 Compare January 12, 2023 01:05
This commit addresses the issue with querying for GVR without version, resourceListInGroup previously returns the resources whenever it finds the match for group name, but instead it should aggregate the resources of all the versions in that group and return the results.
@yharish991 yharish991 force-pushed the fixCapabilityQueryGVR branch from 850e5f2 to 87d3b83 Compare January 12, 2023 01:09
Copy link
Contributor

@danniel1205 danniel1205 left a comment

Choose a reason for hiding this comment

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

the changes lgtm, adding my approval. (But we still need to cleanup before merge)

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/4272/20230112011500/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@rajathagasthya rajathagasthya added the ok-to-merge PRs should be labelled with this before merging label Jan 12, 2023
Copy link
Member

@rajathagasthya rajathagasthya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Please add some tests in a follow up PR.

@rajathagasthya rajathagasthya merged commit 3bb04b9 into vmware-tanzu:main Jan 12, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
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.

6 participants