refactor(x/upgrade): configure go-getter #18470
Conversation
WalkthroughWalkthroughThe recent updates include enhancements to the Changes
TipsChat with CodeRabbit Bot (
|
julienrbrt
added
backport/v0.47.x
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/upgrade/go.mod
Files selected for processing (2)
- x/upgrade/CHANGELOG.md (1 hunks)
- x/upgrade/plan/downloader.go (4 hunks)
Additional comments: 2
x/upgrade/CHANGELOG.md (1)
- 26-34: The changelog update correctly reflects the changes made in the pull request. It is important to ensure that the links to the pull requests are correct and that the descriptions accurately represent the changes made. The entry for the state machine breaking change is particularly important as it signals a significant alteration that could affect users and developers. It is well-documented and includes a link to the relevant pull request for further details.
x/upgrade/plan/downloader.go (1)
- 146-165: The
getFilefunction uses a default HTTP client fromcleanhttp.DefaultClient(). It's important to ensure that this client is properly configured for the use case, especially regarding timeouts and other relevant settings. If the default settings are not appropriate, consider configuring the client with custom settings.
| // NOTE: This functions does not check the provided url for validity. | ||
| func DownloadUpgrade(dstRoot, url, daemonName string) error { | ||
| target := filepath.Join(dstRoot, "bin", daemonName) | ||
|
|
||
| // First try to download it as a single file. If there's no error, it's okay and we're done. | ||
| if err := getter.GetFile(target, url); err != nil { | ||
| if err := getFile(url, target); err != nil { | ||
| // If it was a checksum error, no need to try as directory. | ||
| if _, ok := err.(*getter.ChecksumError); ok { | ||
| return err |
There was a problem hiding this comment.
The comment on line 26 should be updated to reflect the new behavior of the function, which now checks the validity of the URL using the getFile function. This is important for maintainability and clarity of the codebase.
There was a problem hiding this comment.
that's not what getFile does.
There was a problem hiding this comment.
@julienrbrt You're correct, my apologies for the confusion. The getFile function indeed does not perform URL validation; it's responsible for downloading the file using a more secure and configured HTTP client. Thank you for pointing that out. I'll ensure to be more accurate in my future comments.
(cherry picked from commit e64f3bc) # Conflicts: # x/upgrade/CHANGELOG.md # x/upgrade/go.mod
Description
A-18
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
go-getter.Breaking Changes
ParamStore.Refactor