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

Add support for context.Context #893

Merged
merged 2 commits into from Feb 20, 2020
Merged

Add support for context.Context #893

merged 2 commits into from Feb 20, 2020

Conversation

burdiyan
Copy link
Contributor

@burdiyan burdiyan commented Jun 21, 2019

This PR adds support for context.Context. It could help people struggling with passing context to the Run functions, that previously had to wrap cobra.Command initialization inside another function that would accept context.

So now instead of doing:

func createFooCommand(ctx context.Context) *cobra.Command {
    return &cobra.Command{
        Use: "foo",
        Run: func(cmd *cobra.Command, args []string) {
            // Use context here
            // ...
        }
    }
}

you would do:

cmd := &cobra.Command{
    Use: "foo",
    Run: func(cmd *cobra.Command, args []string) {
        ctx := cmd.Context()
        // Use context here
        // ...
    }
}


// Get ctx from somewhere else.
cmd.ExecuteContext(ctx)

This PR is fully backward-compatible, and doesn't require users to use context at all.

It renders this PR #727 (which seems to have conficlits, and also goes against the idiomatic way of using context) irrelevant.

@CLAassistant
Copy link

CLAassistant commented Jun 21, 2019

CLA assistant check
All committers have signed the CLA.

@cpuguy83
Copy link

cpuguy83 commented Jul 1, 2019

This does seem like a decent solution for shoe-horning context support into cobra.Command.

command.go Show resolved Hide resolved
@burdiyan
Copy link
Contributor Author

burdiyan commented Jul 22, 2019

I rebased this PR against origin, so that now the build passes (seems like there were some issues upstream previously). So, hope this can be merged soon.

@burdiyan
Copy link
Contributor Author

burdiyan commented Aug 3, 2019

Fixed merge conflicts again.

@burdiyan
Copy link
Contributor Author

burdiyan commented Aug 22, 2019

Can this be merged?

@korya
Copy link
Contributor

korya commented Aug 23, 2019

The lack of support of context.Context is a huge headache for us. What blocks this PR from getting merged?

PS @burdiyan thanks a lot for the work!

@snovichkov
Copy link

snovichkov commented Oct 11, 2019

@cpuguy83 could you approve and merge this is PR?

@cpuguy83
Copy link

cpuguy83 commented Oct 11, 2019

I am not a maintainer on this project.

@snovichkov
Copy link

snovichkov commented Oct 12, 2019

@cpuguy83 sorry... @spf13 could you approve and merge this is PR?

@gpaul
Copy link

gpaul commented Dec 18, 2019

Bump?

@hairyhenderson
Copy link
Contributor

hairyhenderson commented Feb 18, 2020

This'd be pretty useful - I've just found myself abusing some global variables pretty badly because of the lack of context.Context support 😉

/cc @bep or @eparis maybe?

@umarcor
Copy link
Contributor

umarcor commented Feb 20, 2020

@hairyhenderson, see #959.

@bep bep merged commit 0da0687 into spf13:master Feb 20, 2020
5 checks passed
@hypergig
Copy link

hypergig commented Apr 20, 2020

thank you!

I think something in the root README would be helpful here

@aventurella-honey
Copy link

aventurella-honey commented Aug 5, 2020

Is it possible to update the context in one command Run/PersistentPreRun so that the altered context is what's passed down to the subcommands?

@burdiyan
Copy link
Contributor Author

burdiyan commented Aug 6, 2020

@aventurella-honey I believe it's not possible. Because context is immutable and you'll have to store altered context somewhere to pass it on to the subcommands, and currently the only way to pass the context is to call ExecuteContext.

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.

None yet