Skip to content

Conversation

@mattmoor
Copy link
Contributor

No description provided.

@mattmoor
Copy link
Contributor Author

I can't restart the failed action, but it looks like a transient network issue.

@mattmoor
Copy link
Contributor Author

The downstream windows build passes with this pulled in: https://travis-ci.org/github/google/go-containerregistry/jobs/751069345

@ktock
Copy link
Member

ktock commented Dec 23, 2020

Could you add a comment on the changed line something like "this prevents the separator to be replaced unintentionally"?

@mattmoor
Copy link
Contributor Author

I added a comment roughly describing things, but essentially since we're using / then path.Clean is the right thing to use across all platforms.

filepath.Clean is the right thing to use if we were using filepath.Separator, but I don't think that's right here because the platform we're running on isn't necessarily indicative of the separator we should use.

I have very little experience with Windows containers, but IIRC that container runtime supports / -> \ so that we can consistently use / everywhere. That may be inaccurate, but IDK how much y'all have tested this with windows?

@ktock
Copy link
Member

ktock commented Dec 23, 2020

Thanks for adding the comment!

I added a comment roughly describing things, but essentially since we're using / then path.Clean is the right thing to use across all platforms.

filepath.Clean is the right thing to use if we were using filepath.Separator, but I don't think that's right here because the platform we're running on isn't necessarily indicative of the separator we should use.

I totally agree with you.
Currently, we test only on linux but let's expand the test cases to other platforms once the library gets stable.

@ktock ktock merged commit a9a0c2d into containerd:master Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants