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 an env_prefix derive option for a consistent prefix for environment variables #3221

Open
2 tasks done
gibfahn opened this issue Dec 27, 2021 · 14 comments
Open
2 tasks done
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@gibfahn
Copy link
Contributor

gibfahn commented Dec 27, 2021

Maintainer's notes

  • Proposed fix is to add a App::env_prefix and Arg::env_prefix and is modeled after help_heading
    • App::env_prefixapplies to all future Args and is set during .arg()
    • An explicit Arg::env_prefix takes precedence over App::env_prefix
    • Derives will need to capture and restore state
    • Better than display_order and DeriveDisplayOrder argument positioning #1807 will allow derives to change the App::env_prefix part-way through struct definition
    • We will generate the env variables during _build (env will internally need to be a Cow), leaving help generation and env variable look up unchanged

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Clap Version

v3.0.0-rc.8

Describe your use case

This was previously discussed in TeXitoi/structopt#370.

Basically the request is to be able to allow a specific prefix to be appended to all #[clap(env)] derived environment variable names.

Describe the solution you'd like

Before:

#[derive(Parser)]
pub struct Opts {
    #[clap(long, env = "FOO_CONFIG_DIR")]
    pub config_dir: String,
    #[clap(long, env = "FOO_TEMP_DIR")]
    pub temp_dir: String,
    #[clap(long, env = "FOO_CACHE_DIR")]
    pub cache_dir: String,
    #[clap(long)]
    pub isolated: bool
}

After:

#[derive(Parser)]
#[clap(env_prefix)]
pub struct Opts {
    #[clap(long, env)]
    pub config_dir: String,
    #[clap(long, env)]
    pub temp_dir: String,
    #[clap(long, env)]
    pub cache_dir: String,
    #[clap(long)]
    pub isolated: bool
}

(where a custom prefix could be set by #[clap(env_prefix="foo")], but the default was the #[clap(name)])

Alternatives, if applicable

I don't think there's an alternative solution to manually setting the environment variable for every option that needs it.

There was a suggestion in the above issue about allowing this to be calculated via a function vs a string literal, but I can't think of a good use-case for it, so it seems unnecessary for this feature.

Additional Context

Examples from the above issue:

rust-analyzer looks for RA_LOG var, rustc looks for RUSC_LOG var, sccache looks for SCCACHE_ENDPOINT, SCCACHE_CACHE_SIZE, SCCACHE_REDIS and other....

@gibfahn gibfahn added the C-enhancement Category: Raise on the bar on expectations label Dec 27, 2021
@epage
Copy link
Member

epage commented Dec 27, 2021

Wanted to double check. Did you mean for your "after" to be:

#[derive(Parser)]
#[clap(env_prefix)]
pub struct Opts {
    #[clap(long, env)]
    pub config_dir: String,
    #[clap(long, env)]
    pub temp_dir: String,
    #[clap(long, env)]
    pub cache_dir: String,
    #[clap(long)]
    pub isolated: bool
}
  • #[clap(env)] is being specified
  • I added --isolated to contrast with the use of env

@epage epage added A-derive Area: #[derive]` macro API S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 27, 2021
@epage
Copy link
Member

epage commented Dec 27, 2021

If we do this, we'd need to make clear that env_prefix would not propagate through flatten / subcommand unlike most other attributes that get set. I'm assuming this limitation isn't enough to hold back making it easier to follow common env variable practices.

@gibfahn
Copy link
Contributor Author

gibfahn commented Dec 30, 2021

Wanted to double check. Did you mean for your "after" to be:

Whoops, yes my bad, you'd still specify env. I updated the original example to match your correction.

If we do this, we'd need to make clear that env_prefix would not propagate through flatten / subcommand unlike most other attributes that get set.

Why wouldn't we want it to propagate? I can see that making the user be explicit is probably a better idea, but I could also see mycmd mysubccmd --myarg being converted to $MYCMD_MYSUBCMD_MYARG (for example), and I'm not sure what the issue would be with flatten used outside of subcommands.

I'm assuming this limitation isn't enough to hold back making it easier to follow common env variable practices.

Definitely not an issue to specify env_prefix in more places either way, especially if the default values mostly work.

@epage
Copy link
Member

epage commented Dec 30, 2021

If we do this, we'd need to make clear that env_prefix would not propagate through flatten / subcommand unlike most other attributes that get set.

Why wouldn't we want it to propagate? I can see that making the user be explicit is probably a better idea, but I could also see mycmd mysubccmd --myarg being converted to $MYCMD_MYSUBCMD_MYARG (for example), and I'm not sure what the issue would be with flatten used outside of subcommands.

Sorry I didn't go into more details on this. We can't make derives for different structs interact at build-time. We have to do all of this at runtime.

Our options are

  • Balloon out the derive API for each new piece of information we want to communicate, either breaking compatibility or having the new functions on the trait call into the old ones with defaults
  • Try to figure out which arguments were added during the flatten and modify them accordingly
  • Make this a part of the builder API.

In thinking about this, maybe it could be worthwhile to add this to the builder API.

@epage epage added S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 30, 2021
@gibfahn
Copy link
Contributor Author

gibfahn commented Jan 17, 2022

In thinking about this, maybe it could be worthwhile to add this to the builder API.

Adding this to the builder API would basically mean that #[clap(env)] would call something like .env(default_env!()) the way #[clap(version)] calls .version(crate_version!()) today?

If so that seems reasonable to me.

Sorry I didn't go into more details on this. We can't make derives for different structs interact at build-time. We have to do all of this at runtime.

For what it's worth I think it would be fine to just have #[clap(env)] work for top-level options, and leave it as a future enhancement to deal with propagating.

@epage
Copy link
Member

epage commented Jan 17, 2022

For what it's worth I think it would be fine to just have #[clap(env)] work for top-level options, and leave it as a future enhancement to deal with propagating.

We want to avoid this. Someone refactoring their app from a single Struct to multiple Structs would then get surprised by it breaking.

@epage epage added S-waiting-on-mentor Status: Needs elaboration on the details before doing a 'Call for participation' S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing and removed S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing S-waiting-on-mentor Status: Needs elaboration on the details before doing a 'Call for participation' labels Jan 25, 2022
@epage
Copy link
Member

epage commented Jan 25, 2022

Alright, so the proposal is we add an App::env_prefix.

Questions

So in writing up these questions, I think we've got our answers.

@epage epage added E-medium Call for participation: Experience needed to fix: Medium / intermediate and removed S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Jan 25, 2022
@Stargateur
Copy link

Stargateur commented May 20, 2022

If we do this, we'd need to make clear that env_prefix would not propagate through flatten / subcommand unlike most other attributes that get set. I'm assuming this limitation isn't enough to hold back making it easier to follow common env variable practices.

It's would be amazing that flatten to do it automatically for example I would like:

#[derive(Debug, Args, Deserialize)]
pub struct PostgreSQL {
  #[clap(long, env)]
  pub host: Option<String>,
}

#[derive(Parser, Debug)]
#[clap(author, version, about)]
#[clap(env_prefix)]
pub struct ConfigArgs {
  #[clap(flatten)]
  pub postgresql: PostgreSQL,
}

This mean host would have the env {NAME}_POSTGRESQL_HOST, if it's not possible it would be great to be able to use env_prefix more than one like:

#[derive(Debug, Args, Deserialize)]
#[clap(env_prefix(name, struct_name))]
pub struct PostgreSQL {
  #[clap(long, env)]
  pub host: Option<String>,
}

@epage
Copy link
Member

epage commented May 20, 2022

It's would be amazing that flatten to do it automatically for example I would like:

Note that my later post shifted this from being a derive feature to a builder feature which means it could work across flatten.

benesch added a commit to benesch/materialize that referenced this issue May 30, 2022
Add an `env_prefix` option to our CLI parsing module. This option will
eventually come to clap itself (clap-rs/clap#3221), but for now we can
roll it ourselves using clap's reflection API.

This option will allow us to factor out common CLI options into their
own structs while still having the environment variable prefixes
determined by the root command.
benesch added a commit to benesch/materialize that referenced this issue May 30, 2022
Add an `env_prefix` option to our CLI parsing module. This option will
eventually come to clap itself (clap-rs/clap#3221), but for now we can
roll it ourselves using clap's reflection API.

This option will allow us to factor out common CLI options into their
own structs while still having the environment variable prefixes
determined by the root command.
benesch added a commit to MaterializeInc/materialize that referenced this issue May 30, 2022
Add an `env_prefix` option to our CLI parsing module. This option will
eventually come to clap itself (clap-rs/clap#3221), but for now we can
roll it ourselves using clap's reflection API.

This option will allow us to factor out common CLI options into their
own structs while still having the environment variable prefixes
determined by the root command.
@Maher4Ever
Copy link

@epage Is this planned to be implemented soon?

@epage
Copy link
Member

epage commented Jul 18, 2022

It has the E-medium tag which means the current design is accepted for moving forward. However, my focus is on other area (iterating on the API for reducing binary size, reducing compile times, and improving flexibility). If someone is willing to step up to implement it, I'd be willing to review and merge the PRs.

flihp added a commit to oxidecomputer/dice-util that referenced this issue Nov 10, 2022
Clap doesn't yet support an easy derive mechanism for adding a prefix to
the environment variable names: clap-rs/clap#3221.
If we run into any conflicts / issues we can prefix these manually.
@eighty4

This comment was marked as off-topic.

@epage

This comment was marked as off-topic.

@epage
Copy link
Member

epage commented Aug 1, 2023

Alternatively, #5050 would allow doing this without any other new feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

No branches or pull requests

5 participants