Skip to content

Conversation

vvoland
Copy link
Collaborator

@vvoland vvoland commented Jun 20, 2024

full diff: moby/moby@1a1f3cf...018d93d

Signed-off-by: Paweł Gronowski [email protected]

@vvoland vvoland added this to the 27.0.0 milestone Jun 20, 2024
@vvoland vvoland self-assigned this Jun 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.73%. Comparing base (334421b) to head (d1cb7d4).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5180      +/-   ##
==========================================
- Coverage   61.76%   61.73%   -0.04%     
==========================================
  Files         297      294       -3     
  Lines       20768    20763       -5     
==========================================
- Hits        12828    12817      -11     
- Misses       7024     7027       +3     
- Partials      916      919       +3     

@thaJeztah
Copy link
Member

Hm... looks like the linter isn't happy because we're copying locks somewhere 😬

@vvoland
Copy link
Collaborator Author

vvoland commented Jun 20, 2024

Hmm right, Client became non-copyable due to the atomic.Bool (and implictily sync.Mutex) being added to the struct, and our fakeClient actually embeds the Client by value.

@vvoland vvoland requested review from silvin-lubecki and a team as code owners June 20, 2024 12:24
Comment on lines 17 to 18
type fakeClient struct {
client.Client
client.APIClient
Copy link
Member

Choose a reason for hiding this comment

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

I guess one of the tests is now trying to call the interface somewhere 🤔 (which panics);

#20 61.64 === FAIL: cli/command/container TestNewCreateCommandWithContentTrustErrors (0.00s)
#20 61.64 panic: runtime error: invalid memory address or nil pointer dereference [recovered]
#20 61.64 	panic: runtime error: invalid memory address or nil pointer dereference
#20 61.64 [signal SIGSEGV: segmentation violation code=0x1 addr=0x178 pc=0xc91b5e]
#20 61.64 
#20 61.64 goroutine 77 [running]:
#20 61.64 testing.tRunner.func1.2({0xd84e20, 0x16e8a50})
#20 61.64 	/usr/local/go/src/testing/testing.go:1545 +0x238
#20 61.64 testing.tRunner.func1()
#20 61.64 	/usr/local/go/src/testing/testing.go:1548 +0x397
#20 61.64 panic({0xd84e20?, 0x16e8a50?})
#20 61.64 	/usr/local/go/src/runtime/panic.go:914 +0x21f
#20 61.64 github.com/docker/cli/cli/command/container.(*fakeClient).DaemonHost(0xec5ac1?)
#20 61.64 	<autogenerated>:1 +0x1e
#20 61.64 github.com/docker/cli/cli/command/container.runCreate({0x103af40, 0x1740aa0}, {0x1043c28?, 0xc0003a0240?}, 0x0?, 0xc0003be000, 0xc0003f4b00)
#20 61.64 	/go/src/github.com/docker/cli/cli/command/container/create.go:88 +0xee
#20 61.64 github.com/docker/cli/cli/command/container.NewCreateCommand.func1(0xc0003ae100?, {0xc0003f2320?, 0x4?, 0xec2c34?})
#20 61.64 	/go/src/github.com/docker/cli/cli/command/container/create.go:58 +0x125
#20 61.64 github.com/docker/cli/vendor/github.com/spf13/cobra.(*Command).execute(0xc000004300, {0xc0003f2190, 0x1, 0x1})
#20 61.64 	/go/src/github.com/docker/cli/vendor/github.com/spf13/cobra/command.go:985 +0xabc
#20 61.64 github.com/docker/cli/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0xc000004300)
#20 61.64 	/go/src/github.com/docker/cli/vendor/github.com/spf13/cobra/command.go:1117 +0x3ff
#20 61.64 github.com/docker/cli/vendor/github.com/spf13/cobra.(*Command).Execute(...)
#20 61.64 	/go/src/github.com/docker/cli/vendor/github.com/spf13/cobra/command.go:1041
#20 61.64 github.com/docker/cli/cli/command/container.TestNewCreateCommandWithContentTrustErrors(0x0?)
#20 61.64 	/go/src/github.com/docker/cli/cli/command/container/create_test.go:240 +0x1ec
#20 61.64 testing.tRunner(0xc0003dc340, 0xf39900)
#20 61.64 	/usr/local/go/src/testing/testing.go:1595 +0xff
#20 61.64 created by testing.(*T).Run in goroutine 1
#20 61.64 	/usr/local/go/src/testing/testing.go:1648 +0x3ad

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, thought it wouldn't do that 😅

I'll just change these fakeClient to be passed via a pointer then.

Copy link
Member

Choose a reason for hiding this comment

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

If that works for now, I think that's ok to do.

Perhaps we should look later to see if we could actually embed the interface; technically it currently means that "fake" may "not be so fake" and thus call the actual Client; and I'm not sure if that's intentional everywhere or not 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will open a separate PR that we can work on later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The embedded `client.Client` has mutexes and it shouldn't be copied.

Signed-off-by: Paweł Gronowski <[email protected]>
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

@thaJeztah thaJeztah merged commit 623fcd5 into docker:master Jun 20, 2024
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.

3 participants