Skip to content

command: change drive to lowercase for wsl path - #5445

Merged
thaJeztah merged 1 commit into
docker:masterfrom
jsternberg:lowercase-windows-drive
Sep 18, 2024
Merged

thaJeztah merged 1 commit into
docker:masterfrom
jsternberg:lowercase-windows-drive

Conversation

@jsternberg

Copy link
Copy Markdown
Contributor

- What I did

On Windows, the drive casing doesn't matter outside of WSL. For WSL, the drives are lowercase. When we're producing a WSL path, lowercase the drive letter.

- How I did it

Called strings.ToLower on the drive when creating the WSL path.

- How to verify it

- Description for the changelog

* Use lowercase windows drive letter for WSL metrics path

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

@codecov-commenter

codecov-commenter commented Sep 17, 2024 •

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.75%. Comparing base (a4619f3) to head (9f0a594).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5445   +/-   ##
=======================================
  Coverage   59.75%   59.75%           
=======================================
  Files         345      345           
  Lines       23428    23428           
=======================================
  Hits        13999    13999           
  Misses       8455     8455           
  Partials      974      974           

Comment thread cli/command/telemetry_docker.go
@thaJeztah

thaJeztah commented Sep 17, 2024 •

Copy link
Copy Markdown
Member

Can you add (or update) a test-case in

func TestWslSocketPath(t *testing.T) {
u, err := url.Parse("unix:////./c:/my/file/path")
assert.NilError(t, err)
// Ensure host is empty.
assert.Equal(t, u.Host, "")
// Use a filesystem where the WSL path exists.
fs := fstest.MapFS{
"mnt/c/my/file/path": {},
}
assert.Equal(t, wslSocketPath(u.Path, fs), "/mnt/c/my/file/path")
// Use a filesystem where the WSL path doesn't exist.
fs = fstest.MapFS{
"my/file/path": {},
}
assert.Equal(t, wslSocketPath(u.Path, fs), "")
}
?

(could be a test-table if we want to add more cases to it)

On Windows, the drive casing doesn't matter outside of WSL. For WSL, the
drives are lowercase. When we're producing a WSL path, lowercase the
drive letter.

Co-authored-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Co-authored-by: Laura Brehm <laurabrehm@hey.com>

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@laurazard
laurazard force-pushed the lowercase-windows-drive branch from 9f0a594 to 3472bbc Compare September 18, 2024 10:01
@laurazard
laurazard requested a review from thaJeztah September 18, 2024 10:01
@laurazard

Copy link
Copy Markdown
Member

@thaJeztah can you TAL?

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah
thaJeztah merged commit a5fb752 into docker:master Sep 18, 2024
@jsternberg
jsternberg deleted the lowercase-windows-drive branch September 18, 2024 13:50
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.

4 participants