fix: Rootless error concerning `/var/run/docker.sock` #2181

AndesKrrrrrrrrrrr · 2024-01-30T12:24:39Z

Fixes #724
Fixes #2016

This reverts commit b2a8394.

mergify · 2024-01-30T12:25:13Z

⚠️ The sha of the head commit of this PR conflicts with #2178. Mergify cannot evaluate rules on this PR. ⚠️

codecov · 2024-01-30T12:43:29Z

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (5a80a04) 61.56% compared to head (e8d4192) 61.76%.
Report is 10 commits behind head on master.

Files	Patch %	Lines
pkg/container/docker_socket.go	85.52%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2181      +/-   ##
==========================================
+ Coverage   61.56%   61.76%   +0.20%     
==========================================
  Files          53       54       +1     
  Lines        9002     9078      +76     
==========================================
+ Hits         5542     5607      +65     
- Misses       3020     3029       +9     
- Partials      440      442       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mergify · 2024-01-30T13:06:21Z

@AndesKrrrrrrrrrrr this pull request has failed checks 🛠

AndesKrrrrrrrrrrr · 2024-01-30T13:09:02Z

@ChristopherHX, do you mind taking a look? Tests are passing, but coverage diff is failing; the greater part of the modified files are untested and are CLI. Does test coverage apply here?

ChristopherHX · 2024-01-30T13:22:04Z

coverage diff is failing

My own PR's failing in that metric and requiring tests of old code.

I'm not the owner, converage wasn't configured by me (this check is required to pass, if the owner doesn't bypass the rules).
You can suggest to decrease the diff coverage target here https://github.com/nektos/act/blob/master/codecov.yml, if cplee agree's then we can do it.
Maybe it is possible to exclude the cli, but it seems your unrelated docker_run.go is causing the failure (I suggest to split it out into another PR).

Recently coverage metrics were broken, because it compared the diff with a over 400days old version of act, this allowed 0% diff coverage to pass.

ChristopherHX · 2024-01-30T14:15:24Z

Coverage not affected when comparing 5a80a04...ee5a166

So yes it worked.

ChristopherHX

I guess your change is okish

cmd/root.go

ChristopherHX

found breaking change I don't like to see

cmd/root.go

mergify · 2024-02-01T16:03:28Z

@AndesKrrrrrrrrrrr this pull request has failed checks 🛠

ChristopherHX

Thank you for adding tests, but one of them still fails.

I revoke my veto now

AndesKrrrrrrrrrrr · 2024-02-02T12:41:34Z

I think we should be there... finally ❤️

Your code breaks setting DOCKER_HOST if shouldMount is false.

One thing, though, still bugs me about this comment above. Now that I've refactored shouldMount out, what is the intention here? I added a test (see ¹) to verify this case, but no longer sure what is right. What should we do in this case?

🌮	`DOCKER_HOST`	`--container-daemon-socket`	Socket at common locations?	Expectation
B) What I want	Missing	Missing	Yes	Use sane default socket & host
A) What your comment suggests	Missing	Missing	Yes	Only set `DOCKER_HOST`, not socket

https://github.com/AndesKrrrrrrrrrrr/act/blob/b3566aacc4fd92cd4dd48332ce824a6c46f0031b/pkg/container/docker_socket_test.go#L94 ↩

ChristopherHX · 2024-02-02T13:00:13Z

A is how it was before, because I was talking about things that no longer worked in some revisions
I agree B is better than A (would this has been your initial new behavior I wouldn't have commented about it)

ChristopherHX

Looks good to me, the comments about tests don't have to be addressed as they would still work.

pkg/container/docker_socket_test.go

mergify · 2024-02-05T15:35:26Z

@AndesKrrrrrrrrrrr this pull request has failed checks 🛠

AndesKrrrrrrrrrrr · 2024-02-06T08:51:17Z

Looking at the logs, it seems like unrelated tests are failing.

Passing -> swap test assertions -> other tests fail 🤔

* Use same socket defaulting strategy every time * Always default to DOCKER_HOST * Add more debug logs * Commenting, and massively simplified socket logic * Rever to upstream run_context.go * Fix EACCESS error regarding /opt/hostedtoolcache * Revert "Fix EACCESS error regarding /opt/hostedtoolcache" This reverts commit b2a8394. * Revert CLI debug logs * Move socket and host handling to own function, and simplify logic * Move to container package * Make return be a struct * Write tests to verify functionality * Fix DOCKER_HOST being set to the string "DOCKER_HOST" * Always use struct * Use socketLocation, for DOCKER_HOST and more defaults * Fixup arguments to GetSocketAndHost in test and root.go * Un-struct hasDockerHost * Fixup logic and set hasDockerHost * Minor scoping & variable name change * Move functionality to a new file * Rename corresponding test * Reviewfix * Fix DOCKER_HOST expected * Fix test assertions and add comments * Swap comparison actual, expected * Fixed no-DOCKER_HOST env test * Fixed default socket test * Add test to verify review comments * Add more test for greater test coverage * Consistent comment references * Fix bug found while writing tests * Passing tests * NoMountNoHost testfix * Rename test appropriately * NoMount testfix * Fixed OnlySocket * Swap expected <-> actual in tests --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

AndesKrrrrrrrrrrr added 9 commits January 26, 2024 14:02

Use same socket defaulting strategy every time

cf9e9cb

Always default to DOCKER_HOST

1f43b1f

Add more debug logs

da1907a

Commenting, and massively simplified socket logic

51da536

Rever to upstream run_context.go

06368a5

Merge branch 'master' into master

cabec66

Fix EACCESS error regarding /opt/hostedtoolcache

b2a8394

Revert "Fix EACCESS error regarding /opt/hostedtoolcache"

f87dd97

This reverts commit b2a8394.

Merge branch 'master' into master

037dc45

pull-request-size bot added the size/M label Jan 30, 2024

AndesKrrrrrrrrrrr marked this pull request as ready for review January 30, 2024 13:05

AndesKrrrrrrrrrrr requested a review from a team as a code owner January 30, 2024 13:05

mergify bot added the needs-work Extra attention is needed label Jan 30, 2024

Revert CLI debug logs

ee5a166

nektos deleted a comment from mergify bot Jan 30, 2024

mergify bot removed the needs-work Extra attention is needed label Jan 30, 2024

ChristopherHX reviewed Jan 30, 2024

View reviewed changes

cmd/root.go Outdated Show resolved Hide resolved

mergify bot requested a review from a team January 30, 2024 14:24

ChristopherHX requested changes Jan 30, 2024

View reviewed changes

cmd/root.go Outdated Show resolved Hide resolved

mergify bot requested a review from a team January 30, 2024 16:47

AndesKrrrrrrrrrrr added 4 commits January 31, 2024 14:57

Move socket and host handling to own function, and simplify logic

036e4be

Move to container package

1b5cff9

Make return be a struct

54c62a4

Write tests to verify functionality

a461c62

AndesKrrrrrrrrrrr added 2 commits February 1, 2024 14:54

Fix bug found while writing tests

d82ab58

Passing tests

643fff7

mergify bot added the needs-work Extra attention is needed label Feb 1, 2024

ChristopherHX previously approved these changes Feb 1, 2024

View reviewed changes

ChristopherHX mentioned this pull request Feb 1, 2024

--container-daemon-socket and DOCKER_HOST both required with Podman #2016

Closed

AndesKrrrrrrrrrrr added 4 commits February 2, 2024 11:32

NoMountNoHost testfix

b16e799

Rename test appropriately

0bff7a1

NoMount testfix

9855587

Fixed OnlySocket

b3566aa

AndesKrrrrrrrrrrr dismissed ChristopherHX’s stale review via b3566aa February 2, 2024 12:21

AndesKrrrrrrrrrrr requested a review from ChristopherHX February 2, 2024 12:41

mergify bot removed the needs-work Extra attention is needed label Feb 2, 2024

mergify bot requested a review from a team February 2, 2024 13:31

ChristopherHX previously approved these changes Feb 3, 2024

View reviewed changes

pkg/container/docker_socket_test.go Outdated Show resolved Hide resolved

pkg/container/docker_socket_test.go Outdated Show resolved Hide resolved

Swap expected <-> actual in tests

3e0031f

AndesKrrrrrrrrrrr dismissed ChristopherHX’s stale review via 3e0031f February 5, 2024 13:23

ChristopherHX approved these changes Feb 5, 2024

View reviewed changes

mergify bot added the needs-work Extra attention is needed label Feb 5, 2024

AndesKrrrrrrrrrrr mentioned this pull request Feb 6, 2024

fix: sync DOCKER_HOST with cli parameter #2185

Closed

mergify bot removed the needs-work Extra attention is needed label Feb 6, 2024

cplee approved these changes Feb 6, 2024

View reviewed changes

Merge branch 'master' into fix/docker-socket

e8d4192

mergify bot merged commit f2e65e1 into nektos:master Feb 6, 2024
10 checks passed

AndesKrrrrrrrrrrr deleted the fix/docker-socket branch February 7, 2024 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Rootless error concerning `/var/run/docker.sock` #2181

fix: Rootless error concerning `/var/run/docker.sock` #2181

AndesKrrrrrrrrrrr commented Jan 30, 2024

mergify bot commented Jan 30, 2024

codecov bot commented Jan 30, 2024 •

edited

Loading

mergify bot commented Jan 30, 2024

AndesKrrrrrrrrrrr commented Jan 30, 2024

ChristopherHX commented Jan 30, 2024 •

edited

Loading

ChristopherHX commented Jan 30, 2024

ChristopherHX left a comment

ChristopherHX left a comment

mergify bot commented Feb 1, 2024

ChristopherHX left a comment

AndesKrrrrrrrrrrr commented Feb 2, 2024

ChristopherHX commented Feb 2, 2024

ChristopherHX left a comment •

edited

Loading

mergify bot commented Feb 5, 2024

AndesKrrrrrrrrrrr commented Feb 6, 2024

fix: Rootless error concerning /var/run/docker.sock #2181

fix: Rootless error concerning /var/run/docker.sock #2181

Conversation

AndesKrrrrrrrrrrr commented Jan 30, 2024

mergify bot commented Jan 30, 2024

codecov bot commented Jan 30, 2024 • edited Loading

Codecov Report

mergify bot commented Jan 30, 2024

AndesKrrrrrrrrrrr commented Jan 30, 2024

ChristopherHX commented Jan 30, 2024 • edited Loading

ChristopherHX commented Jan 30, 2024

ChristopherHX left a comment

Choose a reason for hiding this comment

ChristopherHX left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 1, 2024

ChristopherHX left a comment

Choose a reason for hiding this comment

AndesKrrrrrrrrrrr commented Feb 2, 2024

Footnotes

ChristopherHX commented Feb 2, 2024

ChristopherHX left a comment • edited Loading

Choose a reason for hiding this comment

mergify bot commented Feb 5, 2024

AndesKrrrrrrrrrrr commented Feb 6, 2024

fix: Rootless error concerning `/var/run/docker.sock` #2181

fix: Rootless error concerning `/var/run/docker.sock` #2181

codecov bot commented Jan 30, 2024 •

edited

Loading

ChristopherHX commented Jan 30, 2024 •

edited

Loading

ChristopherHX left a comment •

edited

Loading