Skip to content

replace docker cli to docker sdk#1144

Open
nXtCyberNet wants to merge 6 commits intohyperledger:mainfrom
nXtCyberNet:feat/docker_sdk
Open

replace docker cli to docker sdk#1144
nXtCyberNet wants to merge 6 commits intohyperledger:mainfrom
nXtCyberNet:feat/docker_sdk

Conversation

@nXtCyberNet
Copy link
Copy Markdown

@nXtCyberNet nXtCyberNet commented Mar 14, 2026

Refactored Docker interactions to replace CLI-based container operations with the
official Docker Go SDK (github.com/docker/docker v28+). This improves reliability,
reduces dependency on shell execution, and provides better error handling.

fixes #1108

Key changes:

  • Replace os/exec based docker commands with Docker Go SDK for container
    exec, start, and stop operations
  • Introduce DockerManager struct wrapping the SDK client with helper methods:
    ContainerExec, ContainerStart, ContainerStop, and ComposeCommand
  • Implement lazy singleton initialization of DockerManager using sync.Once
  • Retain docker compose orchestration via exec.Command since the Docker SDK
    does not provide native compose support
  • Add resolveComposeBinary to detect docker-compose standalone binaries
    (v1/v2/v5) or docker compose plugin, preferring the standalone binary
  • Fix incorrect nil check on CombinedOutput result ([]byte cannot be nil;
    replaced with length check)
  • Add parseExecArgs to parse CLI-style docker exec arguments, including
    environment variable flags (-e)

Tested:
cd scenario
go test -timeout 30m -tags pkcs11 -v -args ../features/

Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
@nXtCyberNet nXtCyberNet requested a review from a team as a code owner March 14, 2026 17:05
Copy link
Copy Markdown
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Just an initial review pass with some minor comments.

Comment thread scenario/go/docker_sdk.go Outdated
Comment on lines +15 to +17
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/client"
"github.com/docker/docker/pkg/stdcopy"
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.

github.com/docker/docker looks to redirect to github.com/moby/moby. The Docker Engine SDK documentation also refers to github.com/moby/moby/client. We should probably use this current package name unless we need to pin to a previous version for compatibility reasons.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's is done intentionally as the docker compose SDK is pinned to GitHub.com/docker/docker , so initially it's done through cli but in future I need to do it

Comment thread scenario/go/docker_sdk.go Outdated

// NewDockerManager initializes a client compatible with v28+ via API negotiation.
func NewDockerManager() (*DockerManager, error) {
c, err := client.NewClientWithOpts(client.FromEnv, client.WithAPIVersionNegotiation())
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.

In the latest version of the Docker SDK, NewClientWithOpts looks to be deprecated in favour of New. For older versions it is not so will depend on which version is used.

Copy link
Copy Markdown
Author

@nXtCyberNet nXtCyberNet Mar 19, 2026

Choose a reason for hiding this comment

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

Yes, it's deprecated in github.com/moby/moby/client but not in github.com/docker/docker since that version was pinned for compatibility with the Docker Compose SDK.

Comment thread scenario/go/docker_sdk.go Outdated
return dm.cli.ContainerStop(context.Background(), containerName, container.StopOptions{Timeout: &timeout})
}

func (dm *DockerManager) ComposeCommand(action ComposeAction, projectDir, file, projectName string) (string, error) {
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.

It might be cleaner to avoid the ComposeAction argument and have separate ComposeUp and ComposeDown methods. They can still use a private method for the shared implementation.

Comment thread scenario/go/docker_sdk.go Outdated

fmt.Println("\033[1m", ">", binary, strings.Join(args, " "), "\033[0m")

cmd := exec.Command(binary, args...) //#nosec G204
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.

It might be nice if we could use something like the Compose SDK instead of exec CLI commands here too. That could happen in a later change though.

Copy link
Copy Markdown
Author

@nXtCyberNet nXtCyberNet Mar 19, 2026

Choose a reason for hiding this comment

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

Okay!! Thank you, sir, for the review.
I'll refactor this part to use the Compose SDK (instead of exec CLI) in the next commit

@nXtCyberNet
Copy link
Copy Markdown
Author

Hey, thanks again for the feedback!
Just to clarify on the client path and future plans:
Currently, we're intentionally using the pinned github.com/docker/docker client (older compatible version) because it aligns with how Docker Compose was set up for stability in earlier releases, and NewClientWithOpts works perfectly without deprecation warnings there.
We can migrate to the modern github.com/moby/moby paths (and full Compose SDK abstractions) once the SDK/client integration is fully stable on that side — which it isn't quite there yet for all use cases, but Compose v2.40+ (and now v5.x) has been moving in that direction with recent Moby bumps. I'll plan to switch over in an upcoming refactor (maybe next few commits) to use the proper Compose SDK methods instead of exec CLI calls, as you suggested — that should clean things up nicely and avoid any future deprecations.
Let me know if you'd prefer we prioritize that migration sooner!

@nXtCyberNet
Copy link
Copy Markdown
Author

Also, the CI failures were due to not pushing the updated go.mod after adding the dependency. I’ll include the updated module files in the next commit.

Copy link
Copy Markdown
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Thank you for the updates.

One other issue that I think does need to be corrected is to rename scenario/go/docker_sdk.go to something like scenario/go/docker_test.go. This is only used by tests and should not be exposed as a public API so the filename should end in _test.go.

nXtCyberNet and others added 5 commits March 19, 2026 20:59
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
Signed-off-by: Rohan Dev <86916212+nXtCyberNet@users.noreply.github.com>
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
Signed-off-by: Rohan Dev <86916212+nXtCyberNet@users.noreply.github.com>
@nXtCyberNet
Copy link
Copy Markdown
Author

Thank you for the updates.

One other issue that I think does need to be corrected is to rename scenario/go/docker_sdk.go to something like scenario/go/docker_test.go. This is only used by tests and should not be exposed as a public API so the filename should end in _test.go.

Thank you for the suggestion. I’ve renamed scenario/go/docker_sdk.go to scenario/go/docker_test.go as it is only used for tests. I’ve also updated the implementation to use the Docker Compose SDK instead of the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use Docker SDK in Go scenario tests

2 participants