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

Implement SetContext() for passing values between hooks #1118

Closed
wants to merge 3 commits into from
Closed

Implement SetContext() for passing values between hooks #1118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d58c46a
Add SetContext() function to cobra.Command
edufschmidt May 19, 2020
a081152
Remove condition for passing ctx to child command
edufschmidt May 19, 2020
d0e2899
Create TestSetContext() for testing passing context between hooks
edufschmidt May 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions command.go
View file
Open in desktop
Expand Up @@ -216,6 +216,12 @@ func (c *Command) Context() context.Context {
return c.ctx
}

// SetContext replaces the underlying command context so that parent
// commands can pass down values to their subcommands.
func (c *Command) SetContext(ctx context.Context) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this any different from ExecuteContext? With using ExecuteContext, a user could pass in the necessary parent context, and then it is executed on the child sub-command.

Copy link
Author

Choose a reason for hiding this comment

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

If I'm not mistaken (it's been a while since this PR was submitted), by using ExecuteContext you're not able to set the context in the scope of a command so that nested commands have access to the new, modified context. In other words, the context is only passed to the root command, and then it is simply forwarded without modification to any subcommands. If you want parent commands to be able to act as middleware to their children, you need to have a structure that can be modified within the root context, which might not be desirable. That's where SetContext comes in handy. It allows to modify the context at will.

Copy link

Choose a reason for hiding this comment

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

@jpmcb Any update? :) Thanks!

c.ctx = ctx
}

// SetArgs sets arguments for the command. It is set to os.Args[1:] by default, if desired, can be overridden
// particularly useful when testing.
func (c *Command) SetArgs(a []string) {
Expand Down Expand Up @@ -943,9 +949,7 @@ func (c *Command) ExecuteC() (cmd *Command, err error) {

// We have to pass global context to children command
// if context is present on the parent command.
if cmd.ctx == nil {
cmd.ctx = c.ctx
}
cmd.ctx = c.ctx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was the if removed here?

I can imagine, even with the addition of the new SetContext API, there still being the need for this if catch. The way I read this code now is that on every execution, a passed in command will always have it's context set.

This could even be a breaking change if during command execution, the users rely on the cmd.ctx not trickling down to subsequent child commands. This change makes the assumption that all sub-command executions should have the same context as the parent.

Copy link
Author

Choose a reason for hiding this comment

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

Please see #1109 for the motivation behind this change.


err = cmd.execute(flags)
if err != nil {
Expand Down
32 changes: 32 additions & 0 deletions command_test.go
View file
Open in desktop
Expand Up @@ -148,6 +148,38 @@ func TestSubcommandExecuteC(t *testing.T) {
}
}

func TestSetContext(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding a test case for this!! 👍

Copy link
Author

Choose a reason for hiding this comment

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

👍

ctx := context.TODO()

aKey := "akey"
aVal := "aval"

parentRun := func(cmd *Command, args []string) {
ctx := context.WithValue(cmd.Context(), aKey, aVal)
cmd.SetContext(ctx)
}

childRun := func(cmd *Command, args []string) {
if val := cmd.Context().Value(aKey); val != aVal {
t.Errorf(`Context attribute not found in child command. Expected: "%+v". Have: "%+v"`, aVal, val)
}
}

rootCmd := &Command{Use: "root", Run: parentRun, PersistentPreRun: parentRun}
childCmd := &Command{Use: "child", Run: childRun}

rootCmd.AddCommand(childCmd)

if _, err := executeCommandWithContext(ctx, rootCmd, ""); err != nil {
t.Errorf("Root command must not fail: %+v", err)
}

if _, err := executeCommandWithContext(ctx, rootCmd, "child"); err != nil {
t.Errorf("Subcommand must not fail: %+v", err)
}

}

func TestExecuteContext(t *testing.T) {
ctx := context.TODO()

Expand Down