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

volume/local: Fix CIFS urls with spaces, add tests #47194

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Jan 23, 2024

- What I did
Fixed spaces in CIFS address being escaped
Added tests

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

I made a mistake in the last commit - after resolving the IP from the
passed `addr` for CIFS it would still resolve the `device` part.

Apply only one name resolution

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Unescapes the URL to avoid passing an URL encoded address to the kernel.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland force-pushed the volume-cifs-resolve-optout-2 branch from 5ba1e0d to 2508867 Compare January 23, 2024 16:42
@vvoland
Copy link
Contributor Author

vvoland commented Jan 23, 2024

Might want to just merge this one instead of #47193 (it's included in this PR).

@thaJeztah
Copy link
Member

Yeah, I'd be fine with closing the other one.

@thaJeztah
Copy link
Member

These buildkit tests are very flaky recently;

=== FAIL: client TestIntegration (5.59s)
time="2024-01-23T16:52:50Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:36285"
    run.go:266: copied docker.io/amd64/alpine:latest@sha256:25fad2a32ad1f6f510e528448ae1ec69a28ef81916a004d3629874104f8a7f70 to local mirror localhost:36285/library/alpine:latest
time="2024-01-23T16:52:50Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:36285"
    run.go:266: copied docker.io/tonistiigi/test:nolayers to local mirror localhost:36285/tonistiigi/test:nolayers
time="2024-01-23T16:52:51Z" level=info msg="trying next host - response was http.StatusNotFound" host="localhost:36285"
time="2024-01-23T16:52:51Z" level=info msg=request digest="sha256:be691b1535726014cdf3b715ff39361b19e121ca34498a9ceea61ad776b9c215" mediatype=application/vnd.docker.image.rootfs.foreign.diff.tar size=10240 url="https://gist.github.com/cpuguy83/fcf3041e5d8fb1bb5c340915aabeebe0/raw/eebc59ee14939fc38067aa8b8dfbfba053d0af94/base.tar"
    run.go:266: copied docker.io/cpuguy83/buildkit-foreign:latest to local mirror localhost:36285/cpuguy83/buildkit-foreign:latest

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM

@vvoland
Copy link
Contributor Author

vvoland commented Jan 23, 2024

Thanks! I'll go ahead and merge it.

@vvoland vvoland merged commit 0b64499 into moby:master Jan 23, 2024
126 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants