How can I pass context.Context between pre/post-run hooks?

[binary132]

In my pre-run hook, I'd like to set a value (someplace; maybe in the Command?) and have access to it in my Run. This seems like a good use-case for context.Context. Is this supported somehow? I was thinking of setting a Flag value, but that seems awfully hackish, and I really don't want to use a global variable.

[dnephin]

The way I've seen this done is to always wrap the cobra.Command{} in a function, which accepts the context object. I haven't seen context.Context used before, usually it's a more specific type defined by the application. The function wrapping the creation of the cobra.Command accepts an instance of the type, so it exists in the scope for both Run and PreRun.

func newXCommand(context CustomContext) *cobra.Command {
    return &cobra.Command{
        ...
    }
}

In main() you would create a new context and pass it to every function that creates a new command.

[binary132]

What do you think about offering a Context version of the lifecycle functions (Pre / Post) with a Command.Context() context.Context method on Command to retrieve the current Context (or nil)? The user-facing API for that could look a bit like this:

const (
    Response = iota
    Note
)

// preCmd always makes an HTTP request, which all of its children want to use
var preCmd = &cobra.Command {
    // ...
    PersistentPreRunCtx: func(cmd *cobra.Command) context.Context {
        rsp, _ := http.Get("https://google.com")
        return context.WithValue(context.WithTimeout(
            // Response Body must be read within 5 seconds
            cmd.Context(), 5*time.Second,
        ), Response, rsp.Body)
    },
}

// preCmd is the parent of storeCmd
var storeCmd = &cobra.Command {
    // ...
    PreRun: func(cmd *cobra.Command) context.Context {
        return context.WithValue(cmd.Context(), Note, "hi there")
    },
    RunCtx: func(cmd *cobra.Command) context.Context {
        rc := cmd.Context().Value(Response).(io.ReadCloser)
        // db.WriteAll(uuid.NewV4(), rc)
    },
}

Each lifecycle hook, if it returns a Context, could be threaded with something like:

func ThreadCtx(cmd *Command, with context.Context, fs ...func(cmd *Command) context.Context) {
    c := with
    for _, f := range fs {
        select {
        case <-c.Done():
            // fail gracefully
        case err := <-c.Err():
            // fail less gracefully
        default:
            // thread Context through next Command
        }
        if c = f(cmd); c == nil { continue } else { cmd.ctx = c }
    }
}

[flimzy]

The way I've seen this done is to always wrap the cobra.Command{} in a function, which accepts the context object. I haven't seen context.Context used before, usually it's a more specific type defined by the application. The function wrapping the creation of the cobra.Command accepts an instance of the type, so it exists in the scope for both Run and PreRun.

The problem with this approach, is that it requires a mutable context variable, which violates one of the core principles of the 'context' package. This can be done, if you're willing to hack your context beyond recognition, and well outside of best practices and concurrency safety (which may not matter in many CLI apps, anyway).

To illustrate, suppose you want your Pre function to set a context variable foo to 1, and read it later in Run.

If you follow normal context usage, you'd instead call ctx = context.WithValue(ctx, "foo", 1) in Pre -- but then the new ctx is lost by the time Run is called.

To compensate, you must either pass a pointer to ctx (bad practice~) or you set the foo value in the context before calling Pre, and set it to a (mutable) pointer, then let Pre change its value. Both of these have serious drawbacks (which is why they are excluded from normal usage), and IMO should only be used as a very last resort, when you absolutely must use third-party apps that libraries on ctx.

[flimzy]

I think the only way to make this work properly would be to add ctx versions of the various Run commands. And unfortunately, they all need to accept and return ctx, so the new function signatures would be something like:

PersistentPreRunCtx func(ctx context.Context, cmd *Command, args []string) context.Context
PersistentPreRunCtxE func(ctx context.Context, cmd *Command, args []string) (context.Context, error)
PreRunCtx func(ctx context.Context, cmd *Command, args []string) context.Context
PreRunCtxE func(ctx context.Context, cmd *Command, args []string) (context.Context, error)
RunCtx func(ctx context.Context, cmd *Command, args []string) context.Context
RunCtxE func(ctx context.Context, cmd *Command, args []string) (context.Context, error)
PostRunCtx func(ctx context.Context, cmd *Command, args []string) context.Context
PostRunCtxE func(ctx context.Context, cmd *Command, args []string) (context.Context, error)
PersistentPostRunCtx func(ctx context.Context, cmd *Command, args []string) context.Context
PersistentPostRunCtxE func(ctx context.Context, cmd *Command, args []string) (context.Context, error)

(In lieu of the ctx argument passed to each above function, *cobra.Command could have a Context() method that returns the current context, as suggested by @binary132. They are logically the same, so @binary132's approach might be preferable--I haven't considered the implications fully.)

Then cobra would take the ctx returned from each hook function, and pass it the next. So each hook function has to return the context as well (or a new, replacement one, if it has been agumented).
Example:

cmd := &cobra.Command{
    PersistentPreRunCtx: func(ctx context.Context, cmd *cobra.Command, _ []string) context.Context {
        ctx = context.WithValue(ctx, "foo", 1)
        return ctx
    }
}

Logically, something like:

ctx := context.Background()
ctx = cmd.PersistentPreRunCtx(ctx, cmd, args)
ctx = cmd.PreRun(ctx, cmd, args)
ctx = cmd.Run(ctx, cmd, args)

etc, etc

I wonder if anyone else has thoughts about this? Are there implications I've failed to consider? Would a PR be welcome to add this functionality, or has a decision explicitly been made not to support context?

[flimzy]

Another approach, which might seem a bit more dangerous on the surface, but would make for a much smaller change to cobra, and mimics the standard library's http package, would be simply to add two methods to *cobra.Command:

func (c *cobra.Command) Context() context.Context
func (c *cobra.Command) SetContext(context.Context)

This differs slightly from http.Request's approach, in that http.Request.WithContext returns a new request, rather than altering the existing one. That's not an option here, where each hook needs the ability to update context.

It feels a bit icky to me to be replacing a context like that, but I haven't yet imagined a scenario where it would actually break things.

[cpuguy83]

Or provide a new object like cobra.CommandContext which is the same as Command but with context.Context in the method signatures.
cobra.Command could just use a wrapped cobra.CommandContext and use context.Background()

[falzm]

More than 2 years since this issue has been reported: users show interest, several solutions have been proposed and yet the problem remains fixed... Is there a point in keeping it open, aside from frustrating even more people coming here from a Google search?

[cpuguy83]

You keep issues open b/c it's not fixed.
This project has an issue of lack of active maintainers.

That said there is a push to make a 1.0 release, and with that some minimal changes have been merged recently, including support for context: #893

So this issue can be closed, I think.

[github-actions]

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

[c4milo]

I just wanted to add a vote here. Cobra is great for the most part but it is definitely missing a way for us to provide cross cutting pieces of behavior such as a timeout/deadline or a configuration object we loaded from a file. Currently, it sort of force us to define globals (or singletons) that will then be used from within the commands, or to repeat the same logic on every command.

[cpuguy83]

@c4milo Support for context is there now.

[c4milo]

@cpuguy83, it is a bit limited, for instance, I want to receive a timeout value from a flag and set it in the context but i can't do that because the context gets set earlier than flag evaluation. And, there is no way to re-set that context in Cobra.

[cpuguy83]

@c4milo I'm not sure I understand, since you don't need to have a timeout for all commands, just the running command.
So in Run, read the duration value and ctx, _ = context.WithTimeout(ctx, config.Timeout)

[c4milo]

I need the timeout in all commands, which means having to duplicate that code for every one of them. The same is true for any piece of shared code like creating gRPC clients, setting up logging. The alternative, as I said, is defining them as global variables in some package.

If I could overwrite or re-set cmd.Context from a PersistentPreRunE, that will probably do it.

[c4milo]

I'm not the only one struggling through this BTW: https://news.ycombinator.com/item?id=15717715

[cpuguy83]

That's a post from 2017.

I'm not arguing that exactly thing you are trying to do can't be done with cobra. It can't.
But I am trying to say that you don't need to do that if you make an explicit function call in your Run function to setup that timeout. No need for globals.

[c4milo]

That's a post from 2017.

Exactly.

I'm not arguing that exactly thing you are trying to do can't be done with cobra. It can't.
But I am trying to say that you don't need to do that if you make an explicit function call in your Run function to setup that timeout. No need for globals.

and I'm telling you that I will have to do that for a 100 subcommands and duplicate code.

[cpuguy83]

My point is what is the difference between calling "cmd.Context()" and "GetContext(cmd)" in terms of duplication?

I am not a maintainer here, I'm not saying one shouldn't be able to set contexts on the cmd, just someone who knows how difficult it was to get context support in to begin with (with setting one being contentious) offering up an alternative.

[c4milo]

the difference is that instead of writing this once:

ctx, cancel := context.WithTimeout(cmd.Context(), timeout)
defer cancel()

I'm going to have to write it 100 times. It would be more useful if the context can be set after flags are parsed. It is basically the same that is being asked here: #893 (comment)

[cpuguy83]

@c4milo But that's not true.

1. Presumably you'll need the cancel to propagate to your Run step because that's where you need to defer cancel() or else you'll end up canceling the context before you ever use it.
2. You'd still need to write cmd.Context() 100 times anyway. Replacing this logicless code with slightly different logicless code is not a huge difference no matter how many times it needs to be repeated.

[c4milo]

1. Agreed
2. I will still need to write context.WithTimeout(cmd.Context, timeout) 100 times. And that's just a single example, there are more cross-cutting concerns that could be passed to commands as an instance of something initialized in PersistentPreRunE of the root command. Ex: logger, tracer, gRPC client, feature flag management service client, etc.

[marckhouzam]

This has been addressed with #1551 which adds a SetContext() function that provides the flexibility needed.

Thanks for all the great discussion and we hope this will prove helpful.