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 context.Context to cli.Context #840

Merged
merged 8 commits into from
Aug 6, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion context.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
package cli

import (
"context"
"errors"
"flag"
"os"
"os/signal"
"reflect"
"strings"
"syscall"
)

// Context is a type that is passed through to
// each Handler action in a cli application. Context
// can be used to retrieve context-specific args and
// parsed command-line options.
type Context struct {
context.Context
App *App
Command *Command
shellComplete bool
Expand All @@ -24,9 +28,21 @@ type Context struct {
// NewContext creates a new context. For use in when invoking an App or Command action.
func NewContext(app *App, set *flag.FlagSet, parentCtx *Context) *Context {
c := &Context{App: app, flagSet: set, parentContext: parentCtx}

if parentCtx != nil {
if parentCtx.Context != nil {
parentCtx.Context = context.Background()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This has a side effect on the parentCtx passed in, which I as a user would find suprising.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think the intended condition in the if was == nil it seems?

I think this needs tests, as we can't rely on reviewers to prove this works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius this is just to make sure the context is never nil, so that we never panic. Should I remove it and leave the risk up to the user?

Copy link
Contributor

@AudriusButkevicius AudriusButkevicius Aug 6, 2019

Choose a reason for hiding this comment

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

It's not true, the if statement is wrong and it always overrides it if its NOT nil, and leaves it nil otherwise. I think we should not cause side effects on objects given to us, so if its nil, create a new one locally, and then validate this behaviour with a few tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius great catch, that was a mistake on my part. I will change != nil to == nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AudriusButkevicius i also added some propagation tests to ensure the parent context does not get overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to add sigterm tests, but i'm not sure how I can send a sigterm signal to the process, without killing the tests themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think we should not mutate the parent context.

c.Context = parentCtx.Context

Then outside the if (oppose to else):

If c.Context == nil {
  // Create cancel context, spin up signal handler ctx.
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c.Context = parentCtx.Context
c.shellComplete = parentCtx.shellComplete
} else {
ctx, cancel := context.WithCancel(context.Background())
go func() {
defer cancel()
sigs := make(chan os.Signal, 1)
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
<-sigs
}()
c.Context = ctx
}

return c
Expand Down
3 changes: 3 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module gopkg.in/urfave/cli.v2

go 1.12
marwan-at-work marked this conversation as resolved.
Show resolved Hide resolved