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.

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

Implement SetContext() for passing values between hooks #1118

Closed
wants to merge 3 commits into from

Conversation

edufschmidt
Copy link

@CLAassistant
Copy link

CLAassistant commented May 19, 2020

CLA assistant check
All committers have signed the CLA.

@Divya063
Copy link

Divya063 commented Jun 17, 2020

@spf13 could you approve and merge this is PR?

@aaronc
Copy link

aaronc commented Jul 1, 2020

Would be great to see this get merged!

@alexanderbez alexanderbez mentioned this pull request Jul 1, 2020
27 tasks
@jackzampolin
Copy link

Would really love to see this merged cc @eparis !

@oriser
Copy link

oriser commented Aug 18, 2020

I will also be glad to see this merged :)

@github-actions
Copy link

This PR is being marked as stale due to a long period of inactivity

@AWoelfel
Copy link

I just stumbeled on this and would love to see this merged!

@stapelberg
Copy link

@jpmcb could you take a look at this simple PR please? I’m also interested in seeing this merged :)

Thanks!

@@ -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.

👍

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.

@@ -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.

Choose a reason for hiding this comment

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

@jpmcb Any update? :) Thanks!

@dfreilich
Copy link

Any update on this PR? I was recently looking into this problem, and this would help my use case as well (ensuring that contexts are passed appropriately to deeply nested subcommands)

@congon4tor
Copy link

I believe this would also be useful for my use case. I want to initialise a grpc client that is common to all sub commands but some values to initialise it come from flags in the root command. The way I'm trying to achieve this is by having a PersistentPreRun on the root which initialises the grpc client and stores it in the context (with SetContext()). Then access the client from the sub commands with cmd.Context().

@jpmcb jpmcb added this to the Next milestone Jul 1, 2021
@davidmdm
Copy link

I too would really like this to get merged soon. it would allow for parent pre-runs to set values that child commands would need in a way that would really make my api less verbose.

@alexykot
Copy link

alexykot commented Feb 1, 2022

Hmm, seems like there were multiple attempts to fix this, but all PRs are either closed or still dangling. E.g. #1517, #1551.

I didn't look into details of competing solutions to have a preference here, but would be great to merge either of them to have this feature available.

@johnSchnake
Copy link
Collaborator

As part of #1600 I'm going through the backlog and found an issue related to supporting context. We now can get context but can't set the context. This PR seems to close the gap. I'm going to take a look at this and talk with the maintainers and see if we can (or why we can't) get this merged. See #563

@stapelberg
Copy link

Thanks very much! I’m still interested in having this functionality, so this is great news :)

@marckhouzam
Copy link
Collaborator

The ability to set a context has just been merged thanks to #1551. That PR, although opened more recently than this one, was limited to adding the SetContext() function, so we preferred that one.

If you are still interested in addressing #1109, which I don't fully grasp all the possible impacts with respect to backwards-compatibility, please open a new PR just for that issue and we will try to evaluate those impacts.

Thanks for you patience.

hoshsadiq pushed a commit to zulucmd/zulu that referenced this pull request Dec 31, 2022
Basically the same as spf13/cobra#1517 but
uses the `Set<field>` naming convention instead of `WithContext`

Context setting without execution is important because it means
that more design patterns can be achieved.

Currently I am using functional options in a project and I can add
behaviour through functional options as such:
```go
type GrptlOption func(*cobra.Command) error

func WithFileDescriptors(descriptors ...protoreflect.FileDescriptor) GrptlOption {
	return func(cmd *cobra.Command) error {
		err := CommandFromFileDescriptors(cmd, descriptors...)
		if err != nil {
			return err
		}
		return nil
	}
}
```

I've got a lot more options and this pattern allows me to have nice
abstracted pieces of logic that interact with the cobra command.

This Pattern also allows for adding extra information to a call
through `PreRun` functions:

```go

cmd := &cobra.Command{
		Use: "Foobar",
		PersistentPreRun: func(cmd *cobra.Command, args []string) {
			err :=cmd.SetContext(metadata.AppendToOutgoingContext(context.Background(), "pre", "run"))
		},
	}
```

This is a veer nice abstraction and allows for these functions to be
very modular

The issue I'm facing at the moment is that I can't write a nifty
option (something like `WithAuthentication`) because that needs access
to reading and setting the context. Currently I can only read the
context.

Needing to use `ExecuteContext` breaks this abstraction because I need
to run it right at the end.

Merge spf13/cobra#1551

Fixes spf13/cobra#1517
Fixes spf13/cobra#1118
Fixes spf13/cobra#563
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.