Skip to content

command: change drive to lowercase for wsl path#5445

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

command: change drive to lowercase for wsl path#5445
thaJeztah merged 1 commit intodocker:masterfrom
jsternberg:lowercase-windows-drive

Conversation

@jsternberg
Copy link
Contributor

@jsternberg jsternberg commented Sep 17, 2024

- 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
Copy link

codecov-commenter commented Sep 17, 2024 •
edited
Loading

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

@thaJeztah thaJeztah added this to the 28.0.0 milestone Sep 17, 2024
thaJeztah
thaJeztah reviewed Sep 17, 2024
View reviewed changes
@thaJeztah
Copy link
Member

thaJeztah commented Sep 17, 2024 •
edited
Loading

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

cli/cli/command/telemetry_docker_test.go

Lines 11 to 29 in a4619f3

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)

@jsternberg @laurazard
command: change drive to lowercase for wsl path
3472bbc 
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 <[email protected]>
Co-authored-by: Laura Brehm <[email protected]>

Signed-off-by: Jonathan A. Sternberg <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
@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
Member

laurazard commented Sep 18, 2024

@thaJeztah can you TAL?

@laurazard laurazard self-assigned this Sep 18, 2024
@laurazard laurazard mentioned this pull request Sep 18, 2024
Merged
thaJeztah
thaJeztah approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

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
@thaJeztah thaJeztah mentioned this pull request Sep 18, 2024
Closed
@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

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

@laurazard laurazard

Labels

area/metrics/otel area/metrics kind/bugfix PR's that fix bugs platform/windows process/cherry-picked status/2-code-review

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@jsternberg @codecov-commenter @thaJeztah @laurazard