Fix #3032: Remove query parameters in ADD command when the destinatio… #3053
Conversation
|
@JeromeJu is this enough? or should i open new PR? |
|
@JeromeJu sorry there was error linter fixed that the test should passed |
pkg/util/command_util.go
Outdated
| // ExtractFilename extracts the filename from a URL without its query url | ||
| func extractFilename(rawURL string) string { | ||
| // not necessarily return an error, due to parsing ambiguities https://pkg.go.dev/net/url#Parse | ||
| parsedURL, _ := url.Parse(rawURL) |
There was a problem hiding this comment.
I see the comment above but can you explain why we don't surface or bubble up this error? From the url.Parse docs
// The url may be relative (a path, without a host) or absolute
// (starting with a scheme). Trying to parse a hostname and path
// without a scheme is invalid but may not necessarily return an
// error, due to parsing ambiguities.
I believe we should bubble up this error, let me know if that makes sense or there is some issue that occurs when doing this from common use cases. I believe the COPY command value when in this method URLDestinationFilepath should have a scheme so we should respect the error
There was a problem hiding this comment.
Thanks, I thought that the filepath example.com/thefile.txt would return an error since it doesn't have the scheme, but it actually doesn't. Thanks for the comment. I've already fixed it and switched the execution order, with ResolveEnvironmentReplacement being executed before extractFilename, i think thats better to replace the env first.
already rebased also
kindly check
|
Thanks for the PR @prima101112! Left one comment related to bubbling up the |
… when the destination is a directory
…cement better execute before getting the filename
Fixes #3032
problem
ADD https://something?asd dir/filename returned something?asdexpected : and fixed
the filename should be
somethingDescription
fix filename.Base still returning query params
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
Reviewer Notes
Release Notes
Describe any changes here so maintainer can include it in the release notes, or delete this block.