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

TKR Resolver: exact TKR version match (if exists) should be the only result #4271

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

imikushin
Copy link
Contributor

What this PR does / why we need it

When the exact TKR version is specified as the version prefix AND such TKR exists, it should be the only result in the returned resolution result set.

This fixes a problem, where there are TKRs whose versions are longer than the provided exactly matching TKR version.

For example, the provided prefix is v1.22.13+vmware.1-tkg.1. There are two TKRs with versions satisfying this version prefix:

  • v1.22.13+vmware.1-tkg.1,
  • v1.22.13+vmware.1-tkg.1-zshippable.

Of the above two, v1.22.13+vmware.1-tkg.1 must be used, because it is the exact match, and is least surprising for the user specifying v1.22.13+vmware.1-tkg.1 as the version prefix.

Which issue(s) this PR fixes

Fixes #4262

Describe testing done for PR

Updated unit tests with the test case exposing the issue.

Release note

Fixed a problem when resolving TKRs, where there are TKRs whose versions are longer 
than the matching TKR's version.

For example, the provided version prefix is `v1.22.13+vmware.1-tkg.1`. There are two TKRs 
with versions satisfying this prefix:
- `v1.22.13+vmware.1-tkg.1`,
- `v1.22.13+vmware.1-tkg.1-zshippable`.

Of the above, `v1.22.13+vmware.1-tkg.1` will now be returned, because it is the exact match. 
When the version prefix is shorter (e.g. `v1.22.13+vmware.1`), the result will still be 
`v1.22.13+vmware.1-tkg.1-zshippable` because it is considered a "more recent" version from 
the alpha-numerical comparison point of view.

Additional information

Special notes for your reviewer

…result

When the exact TKR version is specified as the version prefix AND such
TKR exists, it should be the only result in the returned resolution
result set.

This fixes a problem, where there are TKRs whose versions are _longer_
than the provided exactly matching TKR version.

For example, the provided prefix is `v1.22.13+vmware.1-tkg.1`. There
are two TKRs with versions satisfying this version prefix:
- `v1.22.13+vmware.1-tkg.1`,
- `v1.22.13+vmware.1-tkg.1-zshippable`.

Of the above two, `v1.22.13+vmware.1-tkg.1` must be used, because it is
the exact match, and is least surprising for the user specifying
`v1.22.13+vmware.1-tkg.1` as the version prefix.

Signed-off-by: Ivan Mikushin <imikushin@vmware.com>
@imikushin imikushin requested a review from a team as a code owner January 11, 2023 22:51
@imikushin imikushin added area/tkr ok-to-merge PRs should be labelled with this before merging labels Jan 11, 2023
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #4271 (42c2bd9) into main (f39341f) will decrease coverage by 0.90%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4271      +/-   ##
==========================================
- Coverage   49.40%   48.50%   -0.91%     
==========================================
  Files         451      481      +30     
  Lines       44755    46875    +2120     
==========================================
+ Hits        22111    22736     +625     
- Misses      20560    22003    +1443     
- Partials     2084     2136      +52     
Impacted Files Coverage Δ
addons/controllers/machine_controller.go 65.65% <0.00%> (-3.04%) ⬇️
cmd/cli/plugin/isolated-cluster/main.go 0.00% <0.00%> (ø)
...in/cluster/get_machinehealthcheck_control_plane.go 11.11% <0.00%> (ø)
...cluster/delete_machinehealthcheck_control_plane.go 16.66% <0.00%> (ø)
cmd/cli/plugin/cluster/create.go 42.22% <0.00%> (ø)
cmd/cli/plugin/cluster/kubeconfig_get.go 8.82% <0.00%> (ø)
cmd/cli/plugin/cluster/get_node_pools.go 10.52% <0.00%> (ø)
cmd/cli/plugin/cluster/list.go 11.36% <0.00%> (ø)
.../isolated-cluster/imgpkginterface/client_imgpkg.go 0.00% <0.00%> (ø)
cmd/cli/plugin/cluster/set_node_pool.go 14.63% <0.00%> (ø)
... and 22 more

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

Copy link
Contributor

@prkalle prkalle 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 @imikushin!

@imikushin imikushin merged commit 42f03f9 into main Jan 12, 2023
@imikushin imikushin deleted the topic/imikushin/tkr-resolve-exact-version branch January 12, 2023 00:54
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
area/tkr cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
3 participants