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.

Sign up for GitHub

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

Jump to bottom

improve passing context #4774

Merged
merged 1 commit into from
Apr 25, 2024
Merged

improve passing context #4774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions cli-plugins/plugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
PersistentPreRunE = func(cmd *cobra.Command, _ []string) error {
var err error
persistentPreRunOnce.Do(func() {
cmdContext := cmd.Context()
// TODO: revisit and make sure this check makes sense
// see: https://github.com/docker/cli/pull/4599#discussion_r1422487271
if cmdContext == nil {
cmdContext = context.TODO()
}
ctx, cancel := context.WithCancel(cmdContext)
ctx, cancel := context.WithCancel(cmd.Context())
cmd.SetContext(ctx)
// Set up the context to cancel based on signalling via CLI socket.
socket.ConnectAndWait(cancel)
Expand Down
18 changes: 18 additions & 0 deletions cmd/docker/builder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import (
var pluginFilename = "docker-buildx"

func TestBuildWithBuilder(t *testing.T) {
ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

testcases := []struct {
name string
context string
Expand Down Expand Up @@ -64,12 +67,16 @@ echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortD
for _, tt := range testcases {
tt := tt
t.Run(tt.name, func(t *testing.T) {
ctx2, cancel2 := context.WithCancel(ctx)
defer cancel2()

if tt.builder != "" {
t.Setenv("BUILDX_BUILDER", tt.builder)
}

var b bytes.Buffer
dockerCli, err := command.NewDockerCli(
command.WithBaseContext(ctx2),
command.WithAPIClient(&fakeClient{}),
command.WithInputStream(discard),
command.WithCombinedStreams(&b),
Expand Down Expand Up @@ -126,6 +133,9 @@ func (c *fakeClient) Ping(_ context.Context) (types.Ping, error) {
}

func TestBuildkitDisabled(t *testing.T) {
ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

t.Setenv("DOCKER_BUILDKIT", "0")

dir := fs.NewDir(t, t.Name(),
Expand All @@ -136,6 +146,7 @@ func TestBuildkitDisabled(t *testing.T) {
b := bytes.NewBuffer(nil)

dockerCli, err := command.NewDockerCli(
command.WithBaseContext(ctx),
command.WithAPIClient(&fakeClient{}),
command.WithInputStream(discard),
command.WithCombinedStreams(b),
Expand Down Expand Up @@ -163,6 +174,9 @@ func TestBuildkitDisabled(t *testing.T) {
}

func TestBuilderBroken(t *testing.T) {
ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

dir := fs.NewDir(t, t.Name(),
fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0o777)),
)
Expand All @@ -171,6 +185,7 @@ func TestBuilderBroken(t *testing.T) {
b := bytes.NewBuffer(nil)

dockerCli, err := command.NewDockerCli(
command.WithBaseContext(ctx),
command.WithAPIClient(&fakeClient{}),
command.WithInputStream(discard),
command.WithCombinedStreams(b),
Expand Down Expand Up @@ -199,6 +214,8 @@ func TestBuilderBroken(t *testing.T) {

func TestBuilderBrokenEnforced(t *testing.T) {
t.Setenv("DOCKER_BUILDKIT", "1")
ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

dir := fs.NewDir(t, t.Name(),
fs.WithFile(pluginFilename, `#!/bin/sh exit 1`, fs.WithMode(0o777)),
Expand All @@ -208,6 +225,7 @@ func TestBuilderBrokenEnforced(t *testing.T) {
b := bytes.NewBuffer(nil)

dockerCli, err := command.NewDockerCli(
command.WithBaseContext(ctx),
command.WithAPIClient(&fakeClient{}),
command.WithInputStream(discard),
command.WithCombinedStreams(b),
Expand Down
3 changes: 2 additions & 1 deletion cmd/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (

func main() {
ctx := context.Background()

dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx))
if err != nil {
fmt.Fprintln(os.Stderr, err)
Expand Down Expand Up @@ -352,7 +353,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error {
// We've parsed global args already, so reset args to those
// which remain.
cmd.SetArgs(args)
err = cmd.Execute()
err = cmd.ExecuteContext(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

take note, instead of passing ctx to the root command through convoluted function calls I've cleaned it up a bit by instead passing context to cobra when we call ExecuteContext. This should only affect the CLI and not any plugins that are run through the CLI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, this was one of the things to look into!

(Still catching up on latest changes in this PR, but yes, this was one where my approach was probably not right)


// If the command is being executed in an interactive terminal
// and hook are enabled, run the plugin hooks.
Expand Down
13 changes: 11 additions & 2 deletions cmd/docker/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"bytes"
"context"
"io"
"os"
"testing"
Expand All @@ -15,8 +16,10 @@ import (

func TestClientDebugEnabled(t *testing.T) {
defer debug.Disable()
ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

cli, err := command.NewDockerCli()
cli, err := command.NewDockerCli(command.WithBaseContext(ctx))
assert.NilError(t, err)
tcmd := newDockerCommand(cli)
tcmd.SetFlag("debug", "true")
Expand All @@ -39,7 +42,13 @@ func runCliCommand(t *testing.T, r io.ReadCloser, w io.Writer, args ...string) e
if w == nil {
w = io.Discard
}
cli, err := command.NewDockerCli(command.WithInputStream(r), command.WithCombinedStreams(w))
ctx, cancel := context.WithCancel(context.TODO())
defer cancel()

cli, err := command.NewDockerCli(
command.WithBaseContext(ctx),
command.WithInputStream(r),
command.WithCombinedStreams(w))
assert.NilError(t, err)
tcmd := newDockerCommand(cli)

Expand Down
1 change: 1 addition & 0 deletions docs/generate/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ func gen(opts *options) error {
Use: "docker [OPTIONS] COMMAND [ARG...]",
Short: "The base command for the Docker CLI.",
}

clientOpts, _ := cli.SetupRootCommand(cmd)
if err := dockerCLI.Initialize(clientOpts); err != nil {
return err
Expand Down
3 changes: 1 addition & 2 deletions e2e/cli-plugins/plugins/nopersistentprerun/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

func main() {
plugin.Run(func(dockerCli command.Cli) *cobra.Command {
cmd := &cobra.Command{
return &cobra.Command{
Use: "nopersistentprerun",
Short: "Testing without PersistentPreRun hooks",
// PersistentPreRunE: Not specified, we need to test that it works in the absence of an explicit call
Expand All @@ -25,7 +25,6 @@ func main() {
return nil
},
}
return cmd
},
manager.Metadata{
SchemaVersion: "0.1.0",
Expand Down
Loading