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.

Sign up for GitHub

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

Jump to bottom

Before function for persistent flags #2027

Open
fjl opened this issue Dec 5, 2024 · 10 comments · May be fixed by #2028
Open

Before function for persistent flags #2027

fjl opened this issue Dec 5, 2024 · 10 comments · May be fixed by #2028
Labels
area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this

Comments

@fjl
Copy link
Contributor

fjl commented Dec 5, 2024 •
edited
Loading

This is with v3.0.0-beta1.01.
I came across this issue while porting our commands in go-ethereum from v2 to v3.

In many of our tools, we use a Before hook on the root command to perform common actions such as setting up logging.
As an example, let's define a flag like this to set the loglevel:

var logLevelFlag = &cli.IntFlag{
    Name: "loglevel",
    Value: 3,
}

The root command includes this flag and a Before hook to enable logging.

var app = &cli.Command{
    Flags: []cli.Flag{
        logLevelFlag,
    },

    Commands: []*cli.Command{
        subCommand,
    },
    
    Before: func (ctx context.Context, cmd *cli.Command) (context.Context, error) {
        // here we would set the loglevel on the default logger
        fmt.Println("level:", cmd.Int(logLevelFlag.Name))
        return ctx, nil
    },
}

var subCommand = &cli.Command{
    Name: "sub",
}

This works as expected when invoking the root command (app) directly:

$ ./app --loglevel=5
level: 5

It also works for subcommands when putting the flag before the command:

$ ./app --loglevel=5 sub
level: 5

But it doesn't work when the flag is at the end:

$ ./app sub --loglevel=5
level: 3  # should be 5

It's unexpected behavior for me, because in v3, flags are supposed to be persistent by default, i.e. they should be applicable in any context. I think of the app.Before hook as a place where I can act on flags, regardless of which subcommand is in effect. But perhaps that's not the case.

A workaround could be to set a Before hook on all subcommands. But it's a messy solution for us, since we have a lot of subcommands.

You can find the full example code here: https://gist.github.com/fjl/06ccd9ae685d91ee98dd05128637a8a5
@fjl fjl added area/v3 relates to / is being considered for v3 status/triage maintainers still need to look into this labels Dec 5, 2024
@dearchap dearchap changed the title your feature title goes here Before function for persistent flags Dec 6, 2024
@dearchap
Copy link
Contributor

dearchap commented Dec 6, 2024

@fjl Yes that is the expected behavior. It seems you have run into one of the unintended consequences of making all flags persistent by default. If we fix this we might break other peoples expectation of how Before works. We probably need to add additional hook methods so that we can achieve this behaviour and also satisfy current expectations. See #1273 . if you have any suggestions I would be glad to look at it.

@fjl fjl linked a pull request Dec 6, 2024 that will close this issue
Open
@fjl
Copy link
Contributor Author

fjl commented Dec 6, 2024

I think it's an inconsistent behavior, anyway. It's weird because the flag is defined at the root command level, but its value is not available for root command.

I have posted PR #2028 to show a possible resolution. Let me know what you think.

@fjl
Copy link
Contributor Author

fjl commented Dec 6, 2024 •
edited
Loading

Let me just iterate again why the current behavior is weird: the purpose of the cli library is creating a command structure. In the end, running the CLI app will resolve a single command, and run its action function. The goal of flag parsing, hooks, etc. is setting up things for this resolved action. In that light, setting up different flag values for different subcommand levels is a strange choice, since only the innermost action will run. For the user, the flag level should not matter. They intend to set the flag to configure their chosen action.

@abitrolly
Copy link
Contributor

because in v3, flags are supposed to be persistent by default, i.e. they should be applicable in any context

Not sure I get it. For me, persistence is the ability to save object into database. How that is connected to the fact that v3 doesn't update flag value before calling Before, is a mystery to me.

@fjl
Copy link
Contributor Author

fjl commented Dec 6, 2024 •
edited
Loading

The term 'persistent flag', in the context of this project, means a flag that is defined in a parent command, but can be given to a subcommand. The opposite is 'local flag'. See #1820

@abitrolly
Copy link
Contributor

Thanks. I used to the concept of "global options", but the ability to pass option from subcommand to subcommand seems a bit of overengineering for me. Who in reality uses that not for global options?

@abitrolly
Copy link
Contributor

I am not alone. :D From #1820 (comment)

This is basically reopening #692 since that was closed due to folks not understanding what persistent flags are.

What unfortunate introduction of terminology. I would rename that to "global" instead of "persistent", and if somebody really needs flag parenting, maybe "inheritable" or "sticky" would be a better name.

@fjl
Copy link
Contributor Author

fjl commented Dec 7, 2024 •
edited
Loading

@abitrolly I understand you don't like the name, but I am not the inventor of it. Just trying to report a bug here.

@dearchap
Copy link
Contributor

dearchap commented Dec 7, 2024

I used the PersistentFlag name from cobra. Global doesnt convey the meaning especially when it is set on subcommands. I dont mind calling it "sticky" instead of "persistent". That should be an easy fix. The more important is how do we handle Before ? As I mentioned I would like to add more hooks . Here are some that come to mind

  1. PreParseFlag
  2. PostParseFlag
  3. Before
  4. Action
  5. After

1 & 2 are executed in the context of the current command/subcommand being parsed. Before/Action/After will be executed only for current executing command .

@abitrolly
Copy link
Contributor

abitrolly commented Dec 8, 2024 •
edited
Loading

@dearchap what happens on each level and when it is used?

"in the context of the current command/subcommand being parsed" is not clear. The elusive "context" may contain so many things that referencing it is like using catch all exception handlers. Correct me if I wrong.

PreParseFlag - the whole tree of commands, subcommands and flags is available, the command to be executed is not chosen, flags contain their default values. Useful for debugging.

PostParseFlag - the command to be executed is chosen, flags contain parsed values. Useful for setting global application state not specific to commands, such as logging, before the chosen command is executed.

BeforeExec - hmm, same as above?
ActionExec - same as above? Or same as below?
AfterExec - can be useful to track accidental flag mutations and for application cleanup.

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

Successfully merging a pull request may close this issue.

Run Before actions after setting up subcommand fjl/cli
3 participants
@fjl @dearchap @abitrolly