cli icon indicating copy to clipboard operation
cli copied to clipboard

Ability to add context values

Open nejtr0n opened this issue 1 year ago • 9 comments

Checklist

  • [x] Are you running the latest v3 release? The list of releases is here.
  • [x] Did you check the manual for your release? The v3 manual is here.
  • [x] Did you perform a search about this feature? Here's the GitHub guide about searching.

What problem does this solve?

In old versions we could add context fields in Before function like this

  Before: func(c *cli.Context) error {
      db := "example"
      c.Context = context.WithValue(c.Context, "db", db)
      return nil
  },

It's useful for example for passing logger through all commands.

Solution description

Add some middleware layer to command with signature

func(ctx context.Context, command *cli.Command) func(ctx context.Context, command *cli.Command)

where we could modify passing ctx

Describe alternatives you've considered

Return back Context field to command

nejtr0n avatar Jun 24 '24 17:06 nejtr0n

I agree context should be available everywhere 🥳

ccoVeille avatar Jun 24 '24 21:06 ccoVeille

@nejtr0n What exactly are you trying to achieve ? when calling rootCmd.Run(ctx, args) you can pass in a ctx of your choice with whatever values you define. All subcmds will have a context with parent context set to that so you should have able to retrieve said values. Do you mean you want to pass in a new context to the Action function for the command based on what you set in Before ? I hadnt really thought about that use case. If you can give me a sample of Before/Action functions of your older code I can see what can be done.

dearchap avatar Jun 27 '24 01:06 dearchap

@bartekpacia I am thinking this would be valuable. What do you think ?

dearchap avatar Jul 07 '24 23:07 dearchap

It seems to be a valid feature request. @nejtr0n, could you show an API you'd like (just like you demonstrated what it looked like in "old"/v2 version of this module)?

bartekpacia avatar Jul 08 '24 11:07 bartekpacia

I just encountered this problem as well and hope that the CLI can have this capability.

Based on my needs, the API usage scenario I hope for:

var name string
cmd := &cli.Command{
	Commands: []*cli.Command{
		{
			Name: "config",
			Flags: []cli.Flag{
				&cli.StringFlag{
					Name:        "name",
					Destination: &name,
				},
			},
			Before: func(ctx context.Context, command *cli.Command) (context.Context, error) {
				switch {
				case strings.HasPrefix(name, "good-"):
					return context.WithValue(ctx, "name", name), nil
				case strings.HasPrefix(name, "bad-"):
					return nil, fmt.Errorf("bad name: %s", name)
				default:
					return context.WithValue(ctx, "name", "unknown-"+name), nil
				}
			},
			Action: func(ctx context.Context, command *cli.Command) error {
				name := ctx.Value("name").(string)
				fmt.Println(name)
				return nil
			},
		},
	},
}

or

var name string
cmd := &cli.Command{
	Commands: []*cli.Command{
		{
			Name: "config",
			Flags: []cli.Flag{
				&cli.StringFlag{
					Name:        "name",
					Destination: &name,
				},
			},
			Before: func(ctx context.Context, command *cli.Command) error {
				switch {
				case strings.HasPrefix(name, "good-"):
					command.AddKey("name", name)
					return nil
				case strings.HasPrefix(name, "bad-"):
					return fmt.Errorf("bad name: %s", name)
				default:
					command.AddKey("name", "unknown"+name)
					return nil
				}
			},
			Action: func(ctx context.Context, command *cli.Command) error {
				name := command.GetValue("name").(string)
				fmt.Println(name)
				return nil
			},
		},
	},
}

Reason:

I hope the function of Before is to validate the Flag and generate a value based on the validation (which could be a struct, a string, etc.), so that Action can directly acquire this value for operation.

Take my current real needs as an example:

var doCtx = &do.DoContext{}

var name string
cmd := &cli.Command{
	Commands: []*cli.Command{
		{
			Name: "config",
			Flags: []cli.Flag{
				&cli.StringFlag{
					Name:        "name",
					Destination: &name,
				},
			},
			Before: func(ctx context.Context, command *cli.Command) error {
				dc, err := do.Setup(name);
				if err != nil {
					return err
				}
				doCtx = dc
				return nil
			},
			Action: func(ctx context.Context, command *cli.Command) error {
				return dc.Start()
			},
		},
	},
}

Personally, I prefer the second option because it won't make destructive changes to v3 (even though it's currently in alpha), and it allows Action to have the same capabilities, so that After can also handle things based on the values set by Action :)

BTW. I am interested in submitting a PR for this feat.

BlackHole1 avatar Jul 09 '24 13:07 BlackHole1

@BlackHole1 Can you submit a PR ?

dearchap avatar Aug 11 '24 23:08 dearchap

@BlackHole1 Can you submit a PR ?

@dearchap Of course, but before submitting a PR, I need to know which one urfave/cli currently prefers?

var name string
cmd := &cli.Command{
	Commands: []*cli.Command{
		{
			Name: "config",
			Flags: []cli.Flag{
				&cli.StringFlag{
					Name:        "name",
					Destination: &name,
				},
			},
			Before: func(ctx context.Context, command *cli.Command) (context.Context, error) {
				switch {
				case strings.HasPrefix(name, "good-"):
					return context.WithValue(ctx, "name", name), nil
				case strings.HasPrefix(name, "bad-"):
					return nil, fmt.Errorf("bad name: %s", name)
				default:
					return context.WithValue(ctx, "name", "unknown-"+name), nil
				}
			},
			Action: func(ctx context.Context, command *cli.Command) error {
				name := ctx.Value("name").(string)
				fmt.Println(name)
				return nil
			},
		},
	},
}
var name string
cmd := &cli.Command{
	Commands: []*cli.Command{
		{
			Name: "config",
			Flags: []cli.Flag{
				&cli.StringFlag{
					Name:        "name",
					Destination: &name,
				},
			},
			Before: func(ctx context.Context, command *cli.Command) error {
				switch {
				case strings.HasPrefix(name, "good-"):
					command.AddKey("name", name)
					return nil
				case strings.HasPrefix(name, "bad-"):
					return fmt.Errorf("bad name: %s", name)
				default:
					command.AddKey("name", "unknown"+name)
					return nil
				}
			},
			Action: func(ctx context.Context, command *cli.Command) error {
				name := command.GetValue("name").(string)
				fmt.Println(name)
				return nil
			},
		},
	},
}

BlackHole1 avatar Aug 12 '24 03:08 BlackHole1

The first one seems better. @urfave/cli want to chime in ? Alternatively we could have Before with the current signature and also BeforeWithContext with new signature. Depends on how we want cli/v3 to evolve. I can understand the need for subcommands to pass new contexts down the hierarchy.

dearchap avatar Aug 13 '24 01:08 dearchap

To add another datapoint, our current CLI has a global timeout flag that we use with context.WithTimeout in Before. Lack of this feature blocks us from migrating to v3.

rykov avatar Aug 21 '24 03:08 rykov

@rykov @nejtr0n Want to test out my PR ?

dearchap avatar Oct 26 '24 16:10 dearchap