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

Replace structopt with Clap v3, and add support for env overrides for all flags #1541

Merged
merged 10 commits into from
Mar 23, 2022
Merged

Replace structopt with Clap v3, and add support for env overrides for all flags #1541

merged 10 commits into from
Mar 23, 2022

Conversation

remoun
Copy link
Contributor

@remoun remoun commented Feb 25, 2022 •
edited
Loading

No description provided.

@remoun remoun self-assigned this Feb 25, 2022
@remoun remoun marked this pull request as draft February 25, 2022 03:05
@remoun remoun changed the title Replace structopt with Clap v3 in most packages, and add support for env overrides Replace structopt with Clap v3, and add support for env overrides for most flags Feb 25, 2022
@remoun remoun changed the title Replace structopt with Clap v3, and add support for env overrides for most flags Replace structopt with Clap v3, and add support for env overrides for all flags Feb 25, 2022
@remoun remoun marked this pull request as ready for review February 25, 2022 23:20
@remoun remoun requested review from a team and jgreat February 25, 2022 23:20
@cbeck88
Copy link
Contributor

cbeck88 commented Feb 25, 2022 •
edited
Loading

Can we also port this to 1.2.0 branch? It might simplify deployment if there isn't config churn?

@jcape
Copy link
Contributor

jcape commented Feb 26, 2022 •
edited
Loading

Can we also port this to 1.2.0 branch? It might simplify deployment if there isn't config churn?

Is there an an actual reason to do that? I'd prefer if we didn't do the block version env rename (IMO we should prefix everything with MC_)

@cbeck88
Copy link
Contributor

cbeck88 commented Feb 26, 2022

I guess I was just trying to think about if this will save jason any work. I thought we were doing this partially to simplify the deployment stuff which currently maps a lot of env's onto flags.

But maybe it doesn't save work, because the deployment is currently working without any of these new env's, so 1.2.0 is fine.
If he wants to simplify things and use env in all the docker / helm etc. that can just go in the master / 1.3 branch?

@remoun remoun requested a review from jcape March 2, 2022 03:32
@jgreat
Copy link
Contributor

jgreat commented Mar 3, 2022

This is awesome and will reduce complexity in the long run, but I don't think we need to backport this into 1.2. We'll need some time to refactor helm charts and deployments to take advantage of not needing to map configs to command line args.

@jcape jcape requested review from a team and removed request for a team March 11, 2022 18:31
jcape
jcape suggested changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

Looks good, but I'd prefer if the test vectors string-as-errors thing was reverted for reasons described in situ.

crypto/x509/test-vectors/src/main.rs Outdated Show resolved Hide resolved
@remoun remoun requested review from jcape and a team March 12, 2022 04:18
jcape
jcape previously approved these changes Mar 14, 2022
View reviewed changes
@jcape jcape requested a review from integral-llc March 14, 2022 18:56
@remoun remoun requested a review from a team March 14, 2022 18:57
cbeck88
cbeck88 previously approved these changes Mar 15, 2022
View reviewed changes
jcape
jcape previously approved these changes Mar 16, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
common/src/node_id.rs Outdated Show resolved Hide resolved
@remoun remoun dismissed stale reviews from jcape and cbeck88 via 1d7ab9f March 16, 2022 22:44
@remoun remoun requested review from jcape and cbeck88 March 16, 2022 22:47
remoun
remoun commented Mar 16, 2022
View reviewed changes
fog/ingest/server/src/config.rs Show resolved Hide resolved
fog/overseer/server/src/lib.rs Outdated Show resolved Hide resolved
Remoun Metyas added 3 commits March 16, 2022 19:24
jcape
jcape previously approved these changes Mar 18, 2022
View reviewed changes
@remoun remoun requested a review from jcape March 22, 2022 21:49
cbeck88
cbeck88 previously approved these changes Mar 22, 2022
View reviewed changes
@remoun remoun requested a review from cbeck88 March 22, 2022 22:00
@remoun remoun merged commit 690112f into mobilecoinfoundation:master Mar 23, 2022
@remoun remoun deleted the replace-structopt branch March 23, 2022 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbeck88 cbeck88 cbeck88 approved these changes

@jcape jcape jcape approved these changes

@integral-llc integral-llc Awaiting requested review from integral-llc

@jgreat jgreat Awaiting requested review from jgreat

Assignees

@remoun remoun

Labels
None yet
Projects
Blockchain Core
Archived in project
Milestone
(2022-Q1) Improve use of rust ecosystem
Development

Successfully merging this pull request may close these issues.
4 participants
@remoun @cbeck88 @jcape @jgreat