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

Ability to add context values #1927

Closed
3 tasks done
nejtr0n opened this issue Jun 24, 2024 · 10 comments · Fixed by #1994
Closed
3 tasks done

Ability to add context values #1927

nejtr0n opened this issue Jun 24, 2024 · 10 comments · Fixed by #1994
Assignees
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this

Comments

@nejtr0n
Copy link

nejtr0n commented Jun 24, 2024

Checklist

  • Are you running the latest v3 release? The list of releases is here.
  • Did you check the manual for your release? The v3 manual is here.
  • 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 nejtr0n added area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this labels Jun 24, 2024
@ccoVeille
Copy link
Contributor

I agree context should be available everywhere 🥳

@dearchap
Copy link
Contributor

@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
Copy link
Contributor

dearchap commented Jul 7, 2024

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

@bartekpacia
Copy link
Member

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)?

@BlackHole1
Copy link
Member

BlackHole1 commented Jul 9, 2024

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.

@dearchap
Copy link
Contributor

@BlackHole1 Can you submit a PR ?

@BlackHole1
Copy link
Member

BlackHole1 commented Aug 12, 2024

@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
			},
		},
	},
}

@dearchap
Copy link
Contributor

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.

@rykov
Copy link

rykov commented Aug 21, 2024

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.

@dearchap
Copy link
Contributor

@rykov @nejtr0n Want to test out my PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants