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

ore/cli: add env_prefix configuration option #12765

Merged
merged 1 commit into from
May 30, 2022

Conversation

benesch
Copy link
Member

@benesch benesch commented 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.

Motivation

  • This PR refactors existing code.

Testing

  • This PR has adequate test coverage / QA involvement has been duly considered.

Release notes

This PR includes the following user-facing behavior changes:

  • n/a

Copy link
Contributor

@andrioni andrioni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo storaged, where the STORAGED_ prefix needs to be removed from each option.

src/storaged/src/main.rs Show resolved Hide resolved
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 benesch force-pushed the clap-env-prefix branch from fd0f146 to 2178ded Compare May 30, 2022 14:15
@benesch benesch enabled auto-merge (rebase) May 30, 2022 14:15
Copy link
Contributor

@teskje teskje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benesch benesch merged commit 1862172 into MaterializeInc:main May 30, 2022
Copy link
Contributor

@guswynn guswynn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is awesome! didn't know this was possible

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

Successfully merging this pull request may close these issues.

4 participants