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

Go spf13/viper like way of configuring commandline behaviour #1566

Closed
bpasson opened this issue Feb 7, 2022 · 13 comments
Closed

Go spf13/viper like way of configuring commandline behaviour #1566

bpasson opened this issue Feb 7, 2022 · 13 comments

Comments

@bpasson
Copy link

bpasson commented Feb 7, 2022

Is there a way to extend PicoCLI to support functionality like https://github.com/spf13/viper does for https://github.com/spf13/cobra?

@remkop
Copy link
Owner

remkop commented Feb 7, 2022

Hi @bpasson, yes, absolutely! 😄

Picocli already has built-in support for a lot of viper-like functionality and has hooks for building your own.
(For example, it has some built-in configuration functionality.)
Can you let me know your specific needs?

@bpasson
Copy link
Author

bpasson commented Feb 8, 2022

@remkop I'm looking for resolving options following the order as below from most important to least important

  • explicit set in application
  • command line flag
  • env variable
  • config file
  • default value

The properties default provider doesn't cover this or am I wrong?

@remkop
Copy link
Owner

remkop commented Feb 8, 2022

I see, for that use case also, I don't think you would need to do much work to get that behaviour.
See https://picocli.info/#_custom_variables

You can define an option as follows:

@Option(names = "--option", defaultValue = "${mydefault:-123}"
int optionValue;

That way, the value will be searched in the following places:

  • command line option (e.g., --option=345)
  • then (if not specified) system property: System.getProperty("mydefault")
  • then (if not defined in sys props) environment variable mydefault
  • then the resource bundle of the command (see https://picocli.info/#_internationalization)
  • and finally the default value 123 if not found in any of the above.

Does that answer your question?

@bpasson
Copy link
Author

bpasson commented Feb 9, 2022

Almost. What I am looking for is probably more in line of:

@CommandLine.Command(valueProvider = CustomValueProvider.class,
        resolveOrder={CommandLine.Resolve.FLAG,CommandLine.Resolve.SYSTEM_PROPERTY,CommandLine.Resolve.ENV,
                CommandLine.Resolve.VALUE_PROVIDER,CommandLine.Resolve.DEFAULT} )
public class SampleCommand implements Runnable {

    @CommandLine.Option(names="--option", defaultValue="123",lookup="optionValue")
    int optionValue;
}

public class CustomValueProvider implements ValueProvider {
    String defaultValue(ArgSpec argSpec) throws Exception;
}

Which would then search in the following places:

  • command line option --option=345
  • then (if not specified) system property: System.getProperty("optionValue")
  • then (if not defined in sys props) environment variable: <TOP_COMMAND>_OPTION_VALUE
  • then (if not defined in env vars) try the valueProvider
  • and finally the default value 123.

The ValueProvider could match the DefaultValueProvider already present in PicoCLI, but I'm not really sure how it works in combination with the defaultValue attribute on @Option.

The addition of the resolve order gives flexibility on which sources are considered and in what order of precedence. Not specifying the resolveOrder would give the default order: flag, system-property, env, value-provider, default.

I used the lookup="" attribute to define what to use for using the sources which are not a flag or defaultValue. In the example I picked a certain way the lookup attribute value is converted into an actual variable for resolving. To make it more flexible a lookupNamingStrategy=."" could be introduced.

Naming of attributes, types and values in my example are probably not the best, but I hope they make clear what I would like to achieve.

What do you think?

@remkop
Copy link
Owner

remkop commented Feb 9, 2022

Users currently cannot combine defaultValueProvider and defaultValue: if the @Command has a defaultValueProvider, then the defaultValue of the individual @Option and @Parameter-annotated members is ignored. I would like to keep things as simple as possible. 😅

About introducing a resolveOrder mechanism, please note that users can already specify the order in the variable by nesting multiple lookups, and use different keys for different sources (system props, env vars, resource bundle). 👍
For example:

${bundle:a:-${env:b:-${sys:c:-X}}}

The above variable is expanded as follows. First, try to find key a in the command’s resource bundle. If a is not found in the resource bundle, get the value of environment variable b. If no environment variable b exists, get the value of system property c. Finally, no system property c exists, the value of the expression becomes X.

One thing though, I just realized that the default values from the command's defaultValueProvider are not interpolated, so variables contained in these values are not resolved.
That would certainly be a nice improvement, thanks for raising this!

If that were implemented, I believe that would meet your requirements, would you agree?

@bpasson
Copy link
Author

bpasson commented Feb 10, 2022

The addition of variable interpolation to the defaultValueProvider would be nice, but it isn't really needed for my use-case. My intention is to use the defaultValueProvider as a configuration file reader to determine option values. I would then no longer have the defaultValue on the individual @Option and @Parameter any more. Keeping a set of defaults for all params in the defaultValueProvider would let me work around this, but that makes it all more prone to errors as it is not directly visible in the code where the @Option and @Parameter is defined.

A couple of remarks from my side.

  • I think it makes a lot of sense to have a valueProvider hook like the defaultValueProvider to be able to provide a value, but as a user I would still want to have a defaultValue on the individual @Option and @Parameter, just in case the defaultValueProvider is unable to determine it.
  • I opted for the resolve order outside of the interpolation mechanism because I only have to specify it once then and not on all options over and over using the interpolation mechanism. The nesting can get pretty unreadable. If the resolve order has a good sensible default like arg > system prop > env > provider > default, most users will never have to change it. It does leave an issue on what key to use for resolving. Interpolation doesn't have this issue obviously.

@remkop
Copy link
Owner

remkop commented Feb 10, 2022

I am still not convinced that specifying default values in both the defaultValueProvider and in the defaultValue on the individual @Option is a good idea. What if the config file says the default value is "AAA" and the code says "BBB"?
From a usability point of view, end users should have the last word, so the config file should "win" and the value in the code should be ignored. So, as soon as the command has a defaultValueProvider, default values in the code become stale and perhaps even misleading...

I did notice that the user manual does not document this fact, that the defaultValueProvider overrides the defaultValue on the individual options/parameters. I will update the documentation, thanks for helping me discover that! 😅

There are pros and cons to the resolveOrder=... annotation you propose, for example, there is a trade-off between reduced readability with nested variables versus having control over the order of resolution separately for each option, control over keys, etc.
But the deciding factor for me is really that picocli already has a mechanism (variable interpolation) and I am not convinced we should introduce another annotation for the same thing. 😅

The default order for resolving variables is exactly as you specified (arg > system prop > env > provider), so once the defaultValueProvider supports variable interpolation I believe this meets your requirements. I raised #1571 for this.

That said, I just released 4.6.3, and it may take a while until the next release.
If you need it sooner, one idea is for you to create your own IDefaultValueProvider implementation. Maybe you want to support a different format than properties, like YAML or JSON for your users. I would be interested to hear how that went for you.

@remkop
Copy link
Owner

remkop commented Feb 11, 2022

FYI my previous statement was incorrect; if the default value provider does not have a default value for a specific option, then the default value in the code (defaultValue annotation or field initial value) is used. If both are present, the value from the default value provider is used.

@bpasson
Copy link
Author

bpasson commented Feb 11, 2022

So if I understand correctly. The default value provided on the annotation is used when the defaultValueProvider doesn't provide a value?

I will create a custom default provider to read and write configuration and possibly to system property and env variable resolution.

Is there a way to disable the default mechanism of system property, env and resource bundle lookup as I'm thinking of handling that in the defaultProvider?

@remkop
Copy link
Owner

remkop commented Feb 11, 2022

So if I understand correctly. The default value provided on the annotation is used when the defaultValueProvider doesn't provide a value?

Yes, that is correct. Sorry for the confusion.

I will create a custom default provider to read and write configuration and possibly to system property and env variable resolution.

Is there a way to disable the default mechanism of system property, env and resource bundle lookup as I'm thinking of handling that in the defaultProvider?

As long as the value that is returned from the defaultValueProvider does not contain any variables (Strings that start with ${ and end with }), then picocli will not do any system property, environment variable or resource bundle lookups, so there should be no need to disable anything.

If the application does need default values that look like variables, then the dollar needs to be escaped with an extra dollar: $${XXX} will result in the literal value ${XXX}.

remkop added a commit that referenced this issue Feb 11, 2022
…ering between default values and default value provider
@remkop
Copy link
Owner

remkop commented Feb 11, 2022

I added this section to the user manual: https://picocli.info/#_variables_in_default_values
Feedback welcome!

@bpasson
Copy link
Author

bpasson commented Feb 11, 2022

I think it's good you clarified it for default values. It is more or less mentioned in chapter 20 Variable Interpolation, but you it's in a spot where you easily overlook the power of it.

Happy to see this conversation lead to improvements for both of us. Keep up the good work. I need to get back to my own community contribution https://github.com/quarkiverse/quarkus-rabbitmq-client to finish some issues 😁

Feel free to close the issue as resolved.

@remkop
Copy link
Owner

remkop commented Feb 12, 2022

Cool, thanks for the confirmation. Good luck with your project, it looks nice (you have my 🌟 star).

@remkop remkop closed this as completed Feb 12, 2022
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

No branches or pull requests

2 participants