Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalise upload path to Unix/URI form on Windows #1211

Merged
merged 8 commits into from
May 15, 2020

Conversation

ticky
Copy link
Contributor

@ticky ticky commented May 1, 2020

This is in an effort to address failures to download on Google Cloud due to the mix of path separators being unsupported.

I have updated the related specs to always expect Unix/URI style paths for the relative file path (Path field) on all platforms.

Given that the uploaders all use the AbsolutePath, which is left unchanged here, this should allow uploads to continue to function, but with more correct URLs on upload. Currently, this will also change the path shown to the user in the console output, API and Buildkite UI, which may or may not be desirable. Further discussion of the implications here is likely needed, and in either case this may be considered a breaking change!

This is now behind an experiment flag, as it would be a breaking change for some people, but likely will become the default in the next major release.

potentially fixes #354, fixes #1159, follows #638 (which did almost exactly the same thing!) and #1113.

See also this post in the forum

@ticky ticky requested a review from a team May 1, 2020 18:02
@ticky ticky marked this pull request as ready for review May 1, 2020 18:06
@ticky
Copy link
Contributor Author

ticky commented May 1, 2020

Given the change in paths used for display, perhaps this change requires a new normalised-path field, which is used for computing the upload URL, but not the URL shown during upload or in the UI. I think this would require the API to accept this field, or some way to filter it out, because the api.Agent struct used to pass around this information is built around that purpose. Alternately, we could shift the path normalisation to the steps which compute the URL, but that requires changes to every upload strategy we implement, whereas this nips it in the bud early.

Happy to discuss, though!

@ticky
Copy link
Contributor Author

ticky commented May 1, 2020

Additionally, should this bit of Artifact Downloader logic also do the opposite, and be using filepath.FromSlash / filepath.ToSlash?

// Convert windows paths to slashes, otherwise we get a literal
// download of "dir/dir/file" vs sub-directories on non-windows agents
if runtime.GOOS != `windows` {
path = strings.Replace(path, `\`, `/`, -1)
}

@petemounce
Copy link
Contributor

This looks good to me. The behaviour that causes difficulty for us is uploading from Windows to then download on not-Windows. It makes sense to me to defend against that at the edge, which here is the upload and download (your follow up comment for on Windows). cc @jamiebrynes7

@ticky ticky force-pushed the normalise-upload-paths-windows branch 2 times, most recently from de11aa9 to 444e28a Compare May 8, 2020 23:45
ticky added 8 commits May 8, 2020 17:23
This is in an effort to address failures to download on Google Cloud due to the mix of path separators being unsupported. At this point, this will also alter the path shown to the user in the console output and Buildkite UI, which may not be desirable. Further discussion is needed.

Also, I expect the tests to fail, but I don't have a Windows Golang setup handy.
@ticky ticky force-pushed the normalise-upload-paths-windows branch from 444e28a to 68901e3 Compare May 9, 2020 00:23
@pda pda self-assigned this May 11, 2020
@sj26
Copy link
Member

sj26 commented May 13, 2020

This looks really good, and I think is a great direction.

I'm going to remove the golang team review request because it looks like @pda is assigned.

@sj26 sj26 requested review from pda and removed request for a team May 13, 2020 23:48
Copy link
Member

@pda pda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @ticky 🙌

@pda pda merged commit efdf306 into master May 15, 2020
@pda pda deleted the normalise-upload-paths-windows branch May 15, 2020 01:41
@jayco
Copy link

jayco commented May 15, 2020

This is awesome @ticky 👌

ticky added a commit that referenced this pull request May 28, 2020
While the upload path normalisation logic added in #1211 worked in unit tests, it turns out the subcommand wasn't actually accepting the experiment flag, only `buildkite-agent start` was. This updates all other subcommands to read the experiment values, even if they don't themselves implement an experiment. This permits experiments such as that added in #1211 to exist within in-job commands such as `artifact upload`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants