Add Command.SetContext

[joshcarp]

Basically the same as #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:

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:

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.

[CLAassistant]

All committers have signed the CLA.

[jpmcb]

Awesome! Thanks for this contribution. I'd love some community feedback as well.

And is it possible you could write some tests for this?

I think this would be great for a v2.0 - will hold till we get some more feedback

[joshcarp]

Hey, added some tests; the main use cases I see are:

• Setting context before a run (as an alternative to cmd.ExecuteContext())
  • This would allow packages to add information to the context using functional options for example
• Setting the context through a PreRun or PersistentPreRun hook

I've put tests for both these cases, also one to ensure that ExecuteContext overwrites any SetContext that was called before

[jackzampolin]

Would love to see this merged soon. Would make parsing config in PersistentPreRunE and setting the values on context much easier!!

[alexykot]

I'd like to have this merged asap as well.

I've started designing my CLI tool and intended to use the context to populate the core data needed by all subcommands, only to find out that there is no legit way right now to update the context after it was passed into the root ExecuteContext() call.

I intend to use it exactly as the second use case above describes - parse and load additional data in the PreRun hook, add it to the context, and pass this enriched context along for all downstream subcommands to use.

It's not possible to do that today, so for now I'll have to do all loading before handing off the execution to cobra. I hope this will be merged quickly and I'll be able to use a more elegant way to load data when appropriate rather than fully upfront.

[marckhouzam]

@johnSchnake WDYT?

[johnSchnake]

I like this PR as its so narrowly scoped. Just needs to fix up the golangci-lint issues and I think this should merge.

[joshcarp]

To respond to the question of "why is this needed?":

Currently to pass around information from pre run hooks a global variable needs to be declared:

func TestFoo(t *testing.T){
	globalVar := "default"
	root := &Command{
		Use: "root",
		PreRun: func(cmd *Command, args []string) {
			globalVar = "not default"
		},
		Run: func(cmd *Command, args []string) {
			fmt.Println(globalVar)
		},
	}
	root.ExecuteContext(context.Background())
}

To avoid using global variables, and to allow for cleaner implementation we can use SetContext instead:

func TestFoo(t *testing.T){
	root := &Command{
		Use: "root",
		PreRun: func(cmd *Command, args []string) {
			ctx := context.WithValue(cmd.Context(), key{}, val)
			cmd.SetContext(ctx)
		},
		Run: func(cmd *Command, args []string) {
			fmt.Println(cmd.Context().Value(key{}))
		},
	}
        ctx := context.WithValue(cmd.Context(), "key", "default")
	root.ExecuteContext(ctx)
}

This greatly simplifies the readability and maintainability of code because everything is stored within the context.Context and no reference to external global variables is made.

I have already simulated this by using a mutable pointer that can be found here: https://github.com/joshcarp/grpctl/blob/main/context.go

I use it to set Authorisation headers which would be a nightmare if I needed to make implementations refer to global variables in my package.

[marckhouzam]

LGTM

The tests are great and give a good understanding of what will now be possible.

[marckhouzam]

Based on the fact that @johnSchnake also thinks this is good (#1551 (comment)), I will merge.

[joshcarp]

@marckhouzam thanks!
Also I forgot to link the issue so this can be closed now: #563

[marckhouzam]

@marckhouzam thanks! Also I forgot to link the issue so this can be closed now: #563

Thanks for following up @joshcarp and thanks for this contribution, it seems it will bring some elegance to the usage of Cobra!

[ashleydavies]

Sorry, this isn't related to this PR directly, so I worry it's an undesirable comment, but hopefully you don't mind too much..!

I really wanted to say that the way y'all manage milestones in this repository (I can see above: "@marckhouzam added this to the 1.5.0 milestone") is tremendously helpful and appreciated. I'm so used to digging into tags and digging around for a commit hash, figuring out what version I needed for this feature was so refreshing.

Little things like that make it so much easier to consume a project, and I imagine sometimes must feel like toil or busywork, so wanted to just say it's really helpful 😄 thank you all!

edit: Also, of course, I'm here because this contribution helped me - thanks @joshcarp! 😄

[marckhouzam]

Well, we never mind such kind comments ❤️
Thanks @ashleydavies !
Credit goes to @jpmcb for setting things up and to @umarcor for keeping use disciplined 👏