🐛 clusterctl: fix goproxy to also return versions for major > 1 #7709

chrischdi · 2022-12-08T10:40:56Z

What this PR does / why we need it:

This PR fixes the goproxy client to also list versions of go modules > v1.
The previously used goproxy endpoint only lists versions of major v1.

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 #7700

Additional info:

The goproxy client will not provide the correct results for go based provider which do not follow go module semantical versioning (e.g. tagging v2.0.0 but not setting go.mod to /v2. xref. If this is considered as a no-go, I would propose to remove the goproxy implementation instead of fixing it. Such providers are always able to set GOPROXY=off to allow clusterctl to directly use the github api.

chrischdi · 2022-12-08T11:18:04Z

That stage should be unrelated

/test pull-cluster-api-e2e-informing-main

vincepri

/lgtm

Should we also fix the code in https://github.com/kubernetes-sigs/cluster-api/blob/main/hack/tools/mdbook/releaselink/releaselink.go ?

We should probably dedupe these two and favor less copying around, wdyt?

ykakarap · 2022-12-09T01:09:01Z

/cc @ykakarap

ykakarap · 2022-12-09T01:10:07Z

/area clusterctl

chrischdi · 2022-12-09T11:54:21Z

/lgtm

Should we also fix the code in https://github.com/kubernetes-sigs/cluster-api/blob/main/hack/tools/mdbook/releaselink/releaselink.go ?

We should probably dedupe these two and favor less copying around, wdyt?

Great point. Did already forget that we've "stolen" that from there :-)
Also adjusted the parameter for aws to show the latest version in the doc.

I moved it to internal/goproxy for now. Happy to get better recommendations 👍
I also added it as separate commit for now to better show the diff to the first commit. Will squash later on.

vincepri · 2022-12-09T15:49:48Z

/assign @ykakarap

ykakarap

The changes to support >v1 versions looks good.

There is an edge case where this might now work. Example when a go modules jumps from v1 to v3 then the current code will not list the v3 versions as it will stop immediately after v2 is not found. This is a very rare case and I dont know think why someone might want to jump a major version. But, I thought I would point this out anyway.

FYI: This is a breaking change in clusterctl's library but given our compatibility notice for clusterctl we should be fine and no further action is needed.

internal/goproxy/goproxy_test.go

internal/goproxy/goproxy.go

ykakarap · 2022-12-09T18:22:25Z

hack/tools/mdbook/releaselink/releaselink.go

@@ -58,36 +56,22 @@ func (l ReleaseLink) Process(input *plugin.Input) error {
 		versionRange := semver.MustParseRange(tags.Get("version"))
 		includePrereleases := tags.Get("prereleases") == "true"

-		repo, err := vcs.RepoRootForImportPath(gomodule, false)
+		scheme, host, err := goproxy.GetSchemeAndHost(os.Getenv("GOPROXY"))


Since now we are reading from GOPROXY env variable we should account for scheme and host being empty strings.

If GOPROXY envvar is set to direct or off we will get an empty scheme and host and the code below will fail.

Added returning an error when one of scheme or host is empty to fail.

chrischdi · 2022-12-12T11:57:26Z

Thanks for reviewiewing.

The changes to support >v1 versions looks good.

There is an edge case where this might now work. Example when a go modules jumps from v1 to v3 then the current code will not list the v3 versions as it will stop immediately after v2 is not found. This is a very rare case and I dont know think why someone might want to jump a major version. But, I thought I would point this out anyway.

Yes great point! That's why I mentioned it in the office hours and also added the note at the PR description 👍

chrischdi · 2022-12-12T12:20:15Z

/test pull-cluster-api-e2e-informing-main

fabriziopandini · 2022-12-12T12:33:42Z

/lgtm
after this merge we should check if go proxy can provide a method for listing available versions

chrischdi · 2022-12-12T12:50:25Z

Squashed both commits to a single one.

killianmuldoon

Looks good - just a couple nits

docs/book/src/clusterctl/commands/init.md

docs/book/src/clusterctl/overview.md

docs/book/src/clusterctl/provider-contract.md

Also: * moves goproxy functionality to an internal package * reuses the package also for releaselink * fixes releaselink references for aws

fabriziopandini · 2022-12-12T13:57:57Z

Thanks @chrischdi for quicky jumping on this issue!
/lgtm
/approve

k8s-ci-robot · 2022-12-12T13:58:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

~~OWNERS~~ [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chrischdi · 2022-12-12T14:11:23Z

/cherry-pick release-1.3

k8s-infra-cherrypick-robot · 2022-12-12T14:11:58Z

@chrischdi: #7709 failed to apply on top of branch "release-1.3":

Applying: clusterctl: fix goproxy to also return versions for major > 1
Using index info to reconstruct a base tree...
M	cmd/clusterctl/client/repository/goproxy.go
M	docs/book/src/clusterctl/commands/init.md
M	docs/book/src/user/quick-start.md
Falling back to patching base and 3-way merge...
Auto-merging docs/book/src/user/quick-start.md
Auto-merging docs/book/src/clusterctl/commands/init.md
Removing cmd/clusterctl/client/repository/goproxy_test.go
CONFLICT (modify/delete): cmd/clusterctl/client/repository/goproxy.go deleted in clusterctl: fix goproxy to also return versions for major > 1 and modified in HEAD. Version HEAD of cmd/clusterctl/client/repository/goproxy.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 clusterctl: fix goproxy to also return versions for major > 1
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot requested review from JoelSpeed and stmcginnis December 8, 2022 10:41

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 8, 2022

vincepri approved these changes Dec 8, 2022

View reviewed changes

k8s-ci-robot assigned vincepri Dec 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2022

k8s-ci-robot requested a review from ykakarap December 9, 2022 01:09

chrischdi force-pushed the pr/clusterctl-fix-goproxy-majors branch 2 times, most recently from 3203984 to 6b6c73b Compare December 9, 2022 11:53

chrischdi force-pushed the pr/clusterctl-fix-goproxy-majors branch from 6b6c73b to 2e5e0c9 Compare December 9, 2022 12:28

k8s-ci-robot assigned ykakarap Dec 9, 2022

ykakarap suggested changes Dec 9, 2022

View reviewed changes

chrischdi force-pushed the pr/clusterctl-fix-goproxy-majors branch from 2e5e0c9 to 8e24add Compare December 12, 2022 11:52

chrischdi force-pushed the pr/clusterctl-fix-goproxy-majors branch from 8e24add to 0deb3a6 Compare December 12, 2022 12:08

k8s-ci-robot assigned fabriziopandini Dec 12, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2022

chrischdi force-pushed the pr/clusterctl-fix-goproxy-majors branch from 0deb3a6 to ad4b17d Compare December 12, 2022 12:50

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2022

killianmuldoon reviewed Dec 12, 2022

View reviewed changes

docs/book/src/clusterctl/commands/init.md Outdated Show resolved Hide resolved

docs/book/src/clusterctl/overview.md Outdated Show resolved Hide resolved

docs/book/src/clusterctl/provider-contract.md Outdated Show resolved Hide resolved

clusterctl: fix goproxy to also return versions for major > 1

128e6fc

Also: * moves goproxy functionality to an internal package * reuses the package also for releaselink * fixes releaselink references for aws

chrischdi force-pushed the pr/clusterctl-fix-goproxy-majors branch from ad4b17d to 128e6fc Compare December 12, 2022 13:36

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2022

k8s-ci-robot merged commit f799638 into kubernetes-sigs:main Dec 12, 2022

k8s-ci-robot added this to the v1.4 milestone Dec 12, 2022

This was referenced Dec 12, 2022

[release-1.3] 🐛 clusterctl: fix goproxy to also return versions for major > 1 #7728

Merged

[release-1.2] 🐛 releaselink: fix goproxy to also return versions for major > 1 #7729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 clusterctl: fix goproxy to also return versions for major > 1 #7709

🐛 clusterctl: fix goproxy to also return versions for major > 1 #7709

chrischdi commented Dec 8, 2022 •

edited

Loading

chrischdi commented Dec 8, 2022

vincepri left a comment

ykakarap commented Dec 9, 2022

ykakarap commented Dec 9, 2022

chrischdi commented Dec 9, 2022

vincepri commented Dec 9, 2022

ykakarap left a comment

ykakarap Dec 9, 2022

chrischdi Dec 12, 2022

chrischdi commented Dec 12, 2022

chrischdi commented Dec 12, 2022

fabriziopandini commented Dec 12, 2022

chrischdi commented Dec 12, 2022

killianmuldoon left a comment

fabriziopandini commented Dec 12, 2022

k8s-ci-robot commented Dec 12, 2022

chrischdi commented Dec 12, 2022

k8s-infra-cherrypick-robot commented Dec 12, 2022

🐛 clusterctl: fix goproxy to also return versions for major > 1 #7709

🐛 clusterctl: fix goproxy to also return versions for major > 1 #7709

Conversation

chrischdi commented Dec 8, 2022 • edited Loading

chrischdi commented Dec 8, 2022

vincepri left a comment

Choose a reason for hiding this comment

ykakarap commented Dec 9, 2022

ykakarap commented Dec 9, 2022

chrischdi commented Dec 9, 2022

vincepri commented Dec 9, 2022

ykakarap left a comment

Choose a reason for hiding this comment

ykakarap Dec 9, 2022

Choose a reason for hiding this comment

chrischdi Dec 12, 2022

Choose a reason for hiding this comment

chrischdi commented Dec 12, 2022

chrischdi commented Dec 12, 2022

fabriziopandini commented Dec 12, 2022

chrischdi commented Dec 12, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

fabriziopandini commented Dec 12, 2022

k8s-ci-robot commented Dec 12, 2022

chrischdi commented Dec 12, 2022

k8s-infra-cherrypick-robot commented Dec 12, 2022

chrischdi commented Dec 8, 2022 •

edited

Loading