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

fix: try finding a socket, otherwise fail, respect user choice #1745

Merged
merged 3 commits into from
Apr 23, 2023

Conversation

catthehacker
Copy link
Member

Fixes #1658

@catthehacker catthehacker requested a review from a team as a code owner April 19, 2023 22:06
@github-actions
Copy link
Contributor

github-actions bot commented Apr 19, 2023

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 2 0 0.03s
✅ REPOSITORY gitleaks yes no 2.44s
✅ REPOSITORY git_diff yes no 0.0s
✅ REPOSITORY secretlint yes no 1.31s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #1745 (24298dd) into master (4989f44) will increase coverage by 1.06%.
The diff coverage is 66.12%.

@@            Coverage Diff             @@
##           master    #1745      +/-   ##
==========================================
+ Coverage   61.22%   62.28%   +1.06%     
==========================================
  Files          46       48       +2     
  Lines        7141     7576     +435     
==========================================
+ Hits         4372     4719     +347     
- Misses       2462     2527      +65     
- Partials      307      330      +23     
Impacted Files Coverage Δ
pkg/common/outbound_ip.go 0.00% <0.00%> (ø)
pkg/container/docker_cli.go 82.23% <ø> (ø)
pkg/container/docker_logger.go 52.08% <ø> (ø)
pkg/container/docker_pull.go 33.33% <ø> (ø)
pkg/container/docker_run.go 13.58% <0.00%> (-0.01%) ⬇️
pkg/container/docker_volume.go 0.00% <ø> (ø)
pkg/container/file_collector.go 37.30% <0.00%> (ø)
pkg/container/host_environment.go 0.00% <0.00%> (ø)
...ontainer/linux_container_environment_extensions.go 23.07% <0.00%> (-1.25%) ⬇️
pkg/exprparser/functions.go 66.32% <0.00%> (-1.04%) ⬇️
... and 22 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team April 19, 2023 22:31
Comment on lines +130 to +146
// returns socket path or false if not found any
func socketLocation() (string, bool) {
if dockerHost, exists := os.LookupEnv("DOCKER_HOST"); exists {
return dockerHost, true
}

for _, p := range commonSocketPaths {
if _, err := os.Lstat(os.ExpandEnv(p)); err == nil {
if strings.HasPrefix(p, `\\.\`) {
return "npipe://" + os.ExpandEnv(p), true
}
return "unix://" + os.ExpandEnv(p), true
}
}

return "", false
}
Copy link
Member

@wolfogre wolfogre Apr 20, 2023

Choose a reason for hiding this comment

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

How about:

Suggested change
// returns socket path or false if not found any
func socketLocation() (string, bool) {
if dockerHost, exists := os.LookupEnv("DOCKER_HOST"); exists {
return dockerHost, true
}
for _, p := range commonSocketPaths {
if _, err := os.Lstat(os.ExpandEnv(p)); err == nil {
if strings.HasPrefix(p, `\\.\`) {
return "npipe://" + os.ExpandEnv(p), true
}
return "unix://" + os.ExpandEnv(p), true
}
}
return "", false
}
// socketLocation returns socket path or false if not found any
func socketLocation() (string, bool) {
if dockerHost, exists := os.LookupEnv("DOCKER_HOST"); exists {
return dockerHost, true
}
for _, p := range commonSocketPaths {
p = os.ExpandEnv(p)
// `os.Stat` to find a working file, not `os.Lstate`, or the mode could be `ModeSymlink`
if info, err := os.Stat(p); err == nil {
switch info.Mode().Type() {
case os.ModeSocket:
return "unix://" + p, true
case os.ModeNamedPipe:
return "npipe://" + p, true
default:
return p, true
}
}
}
return "", false
}

@mergify mergify bot requested a review from a team April 20, 2023 02:12
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team April 20, 2023 02:22
ChristopherHX
ChristopherHX previously approved these changes Apr 20, 2023
Copy link
Contributor

@ChristopherHX ChristopherHX left a comment

Choose a reason for hiding this comment

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

I prefer the suggestions of wolfogre, but that's no reason for me to not approve your PR.

Windows works, however I was unshure that npipe://\\.\pipe\docker_engine is accepted by docker due to mixed \ and /.

@@ -118,6 +118,33 @@ func configLocations() []string {
}
}

var commonSocketPaths = []string{
Copy link

Choose a reason for hiding this comment

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

@jpdrsn claims ~/.rd/docker.sock is also one of the possible locations.

Co-authored-by: Jason Song <[email protected]>
Co-authored-by: Jason Song <[email protected]>
@mergify mergify bot requested a review from a team April 21, 2023 17:25
@mergify mergify bot merged commit b6718fd into master Apr 23, 2023
@mergify mergify bot deleted the fix/socket-path branch April 23, 2023 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants