🌱 Remove redirection handling in repository_github client #7951

chrischdi · 2023-01-19T13:06:12Z

The passed http.DefaultClient already handles redirects inside go-githubs DownloadReleaseAsset.

What this PR does / why we need it:

This PR simplifies the logic for the function downloadFilesFromRelease in cmd/clusterctl/client/repository/repository_github.go .

Passing nil instead of http.DefaultClient in https://github.com/chrischdi/cluster-api/blob/4fee5f49b81e0eee228a4e1968bb00843564eeaa/cmd/clusterctl/client/repository/repository_github.go#L381 would result in returning an error instead of manually resolving the redirect.

However this should only ever be the case if either:

http.DefaultClient changes its behaviour to not follow redirects anymore
go-github changes its behaviour to not follow redirects anymore
We do not pass a redirects compatible client anymore (e.g. nil)

All three cases should get catched at the added unit test.

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

chrischdi · 2023-01-19T13:06:58Z

cmd/clusterctl/client/repository/repository_github.go

+			retryError = errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset")
+			return true, nil


Could also be

Suggested change

retryError = errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset")

return true, nil

return false, errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset")

but would then require to handle the error in L378.

We could follow the pattern we have above:

retryError = errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset") return false, retryError

chrischdi · 2023-01-19T14:00:54Z

/test help

k8s-ci-robot · 2023-01-19T14:00:57Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-26-latest-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-ipv6-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-test-mink8s-main
pull-cluster-api-verify-main

In response to this:

/test help

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.

chrischdi · 2023-01-19T14:01:19Z

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

cmd/clusterctl/client/repository/repository_github_test.go

sbueringer · 2023-01-19T14:26:13Z

cmd/clusterctl/client/repository/repository_github.go

+			retryError = errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset")
+			return true, nil


We could follow the pattern we have above:

retryError = errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset") return false, retryError

sbueringer · 2023-01-19T15:38:05Z

Thx!

/lgtm
/assign @ykakarap

k8s-ci-robot · 2023-01-19T15:38:12Z

LGTM label has been added.

Git tree hash: 492f0ff49e8228dc6646185f905b626275c23a9d

chrischdi · 2023-01-19T16:08:33Z

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

ykakarap

Thank you for working on this!

Overall LGTM. Just one comment.

cmd/clusterctl/client/repository/repository_github.go

The passed http.DefaultClient already handles redirects inside go-githubs DownloadReleaseAsset.

chrischdi · 2023-01-20T13:20:32Z

Forgot to revert nil when testing the error handling 😂

sbueringer · 2023-01-20T13:45:35Z

Thank you!

/lgtm

/assign @fabriziopandini
for a final sanity check :)

k8s-ci-robot · 2023-01-20T13:45:40Z

LGTM label has been added.

Git tree hash: 617f820ef94fe3dccf404cf94cd42fcf5ccdb998

fabriziopandini · 2023-01-20T16:09:56Z

@chrischdi thanks for the cleanup!
/lgtm
/approve

k8s-ci-robot · 2023-01-20T16:10:06Z

[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

sbueringer · 2023-01-20T16:46:37Z

/retest

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 19, 2023

k8s-ci-robot requested review from JoelSpeed and sbueringer January 19, 2023 13:06

chrischdi commented Jan 19, 2023

View reviewed changes

chrischdi changed the title ~~Remove redirection handling in repository_github client~~ 🌱 Remove redirection handling in repository_github client Jan 19, 2023

sbueringer reviewed Jan 19, 2023

View reviewed changes

chrischdi force-pushed the pr-repository-client-remove-redirect-handling branch from 4fee5f4 to 1211850 Compare January 19, 2023 15:17

k8s-ci-robot assigned ykakarap and sbueringer Jan 19, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2023

ykakarap reviewed Jan 20, 2023

View reviewed changes

cmd/clusterctl/client/repository/repository_github.go Outdated Show resolved Hide resolved

chrischdi force-pushed the pr-repository-client-remove-redirect-handling branch from 1211850 to 9b77a34 Compare January 20, 2023 09:00

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2023

k8s-ci-robot requested review from sbueringer and ykakarap January 20, 2023 09:00

sbueringer reviewed Jan 20, 2023

View reviewed changes

cmd/clusterctl/client/repository/repository_github.go Show resolved Hide resolved

chrischdi force-pushed the pr-repository-client-remove-redirect-handling branch from 9b77a34 to 8144820 Compare January 20, 2023 12:48

sbueringer reviewed Jan 20, 2023

View reviewed changes

cmd/clusterctl/client/repository/repository_github.go Outdated Show resolved Hide resolved

Remove redirection handling in repository_github client

5d805b5

The passed http.DefaultClient already handles redirects inside go-githubs DownloadReleaseAsset.

chrischdi force-pushed the pr-repository-client-remove-redirect-handling branch from 8144820 to 5d805b5 Compare January 20, 2023 13:20

k8s-ci-robot assigned fabriziopandini Jan 20, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2023

k8s-ci-robot merged commit 7d3355a into kubernetes-sigs:main Jan 20, 2023

k8s-ci-robot added this to the v1.4 milestone Jan 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Remove redirection handling in repository_github client #7951

🌱 Remove redirection handling in repository_github client #7951

chrischdi commented Jan 19, 2023

chrischdi Jan 19, 2023

sbueringer Jan 19, 2023

chrischdi Jan 19, 2023

sbueringer Jan 19, 2023

chrischdi commented Jan 19, 2023

k8s-ci-robot commented Jan 19, 2023

chrischdi commented Jan 19, 2023

sbueringer Jan 19, 2023

sbueringer commented Jan 19, 2023

k8s-ci-robot commented Jan 19, 2023

chrischdi commented Jan 19, 2023 •

edited

Loading

ykakarap left a comment

chrischdi commented Jan 20, 2023

sbueringer commented Jan 20, 2023 •

edited

Loading

k8s-ci-robot commented Jan 20, 2023

fabriziopandini commented Jan 20, 2023

k8s-ci-robot commented Jan 20, 2023

sbueringer commented Jan 20, 2023

		retryError = errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset")
		return true, nil

	retryError = errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset")
	return true, nil
	return false, errors.New("unexpected redirect: http.DefaultClient should be able to handle redirects in DownloadReleaseAsset")

🌱 Remove redirection handling in repository_github client #7951

🌱 Remove redirection handling in repository_github client #7951

Conversation

chrischdi commented Jan 19, 2023

chrischdi Jan 19, 2023

Choose a reason for hiding this comment

sbueringer Jan 19, 2023

Choose a reason for hiding this comment

chrischdi Jan 19, 2023

Choose a reason for hiding this comment

sbueringer Jan 19, 2023

Choose a reason for hiding this comment

chrischdi commented Jan 19, 2023

k8s-ci-robot commented Jan 19, 2023

chrischdi commented Jan 19, 2023

sbueringer Jan 19, 2023

Choose a reason for hiding this comment

sbueringer commented Jan 19, 2023

k8s-ci-robot commented Jan 19, 2023

chrischdi commented Jan 19, 2023 • edited Loading

ykakarap left a comment

Choose a reason for hiding this comment

chrischdi commented Jan 20, 2023

sbueringer commented Jan 20, 2023 • edited Loading

k8s-ci-robot commented Jan 20, 2023

fabriziopandini commented Jan 20, 2023

k8s-ci-robot commented Jan 20, 2023

sbueringer commented Jan 20, 2023

chrischdi commented Jan 19, 2023 •

edited

Loading

sbueringer commented Jan 20, 2023 •

edited

Loading