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 env variable to cmd flags #9040

Merged
merged 12 commits into from
Apr 13, 2021
Merged

Add env variable to cmd flags #9040

merged 12 commits into from
Apr 13, 2021

Conversation

cyberbono3
Copy link
Contributor

@cyberbono3 cyberbono3 commented Apr 1, 2021

Description

Added v.AutomaticEnv() to Viper field of client.Context.
Implemented various test cases to test priority rules flag > env var > config for client config.

closes: #8179


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

client/config/cmd_test.go Outdated Show resolved Hide resolved
client/config/cmd_test.go Outdated Show resolved Hide resolved
client/config/cmd_test.go Outdated Show resolved Hide resolved
client/config/cmd_test.go Outdated Show resolved Hide resolved
client/config/cmd_test.go Outdated Show resolved Hide resolved
client/config/cmd_test.go Outdated Show resolved Hide resolved
@cyberbono3
Copy link
Contributor Author

cyberbono3 commented Apr 6, 2021

@AmauryM Could you leave your feedback, please?

client/config/cmd_test.go Outdated Show resolved Hide resolved
client/config/config_test.go Outdated Show resolved Hide resolved
client/config/config_test.go Show resolved Hide resolved
client/config/config_test.go Outdated Show resolved Hide resolved
client/config/cmd_test.go Outdated Show resolved Hide resolved
client/config/config_test.go Outdated Show resolved Hide resolved
@cyberbono3 cyberbono3 requested a review from alessio April 7, 2021 18:02
@cyberbono3 cyberbono3 marked this pull request as ready for review April 7, 2021 18:04
@cyberbono3 cyberbono3 self-assigned this Apr 7, 2021
@cyberbono3 cyberbono3 force-pushed the cyberbono3/8179-add-env-vars branch from 30333f2 to 3434789 Compare April 7, 2021 21:00
@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #9040 (8230297) into master (cf7b03e) will decrease coverage by 0.03%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9040      +/-   ##
==========================================
- Coverage   58.83%   58.79%   -0.04%     
==========================================
  Files         580      583       +3     
  Lines       32648    32750     +102     
==========================================
+ Hits        19208    19255      +47     
- Misses      11174    11218      +44     
- Partials     2266     2277      +11     
Impacted Files Coverage Δ
client/context.go 54.86% <0.00%> (-0.99%) ⬇️
simapp/simd/cmd/root.go 61.71% <100.00%> (ø)
client/config/toml.go 55.55% <0.00%> (ø)
client/config/config.go 53.33% <0.00%> (ø)
client/config/cmd.go 40.38% <0.00%> (ø)

@amaury1093 amaury1093 added A:automerge Automatically merge PR once all prerequisites pass. backport/0.42.x (Stargate) labels Apr 8, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Nice!

@amaury1093 amaury1093 removed the A:automerge Automatically merge PR once all prerequisites pass. label Apr 8, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

actually, before auto-merging, let me just test manually on the CLI too

client/context.go Outdated Show resolved Hide resolved
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

tACK.

i tested:

# setting manually WithViper("ABC")
ABC_NODE=localhost:1 simd q staking validators

simapp/simd/cmd/root.go Outdated Show resolved Hide resolved
@amaury1093
Copy link
Contributor

@cyberbono3 Could you fix the lint errors?

@cyberbono3
Copy link
Contributor Author

cyberbono3 commented Apr 12, 2021

@cyberbono3 Could you fix the lint errors?

Sure. I have fixed some of them.

However, I still face some Issues:

  1. I have no idea why TestPaginatedVotesQuery fails.
  2. This no file shown here that has to be gofmted.

@amaury1093
Copy link
Contributor

I have no idea why TestPaginatedVotesQuery fails.

It's flaky, see #9010

This no file shown here that has to be gofmted.

See https://github.com/cosmos/cosmos-sdk/pull/9040/files#diff-146f73b0327c426deb3126e44f9234c7a6fe190c1bcb4c98ab8366b3485922edR116

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 13, 2021
@mergify mergify bot merged commit a465ae1 into master Apr 13, 2021
@mergify mergify bot deleted the cyberbono3/8179-add-env-vars branch April 13, 2021 17:32
@amaury1093
Copy link
Contributor

@mergify io backport release/v0.42.x

@mergify
Copy link
Contributor

mergify bot commented May 12, 2021

Sorry but I didn't understand the command. Please consult the commands documentation 📚.

Hey, I reacted but my real name is @Mergifyio

@amaury1093
Copy link
Contributor

@Mergifyio backport release/v0.42.x

mergify bot pushed a commit that referenced this pull request May 12, 2021
* first draft

* add tests in config_test package

* refactored, all tests pass, r4r

* ready for review

* add envPrefix

* Update simapp/simd/cmd/root.go

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

* fix linter issues

* minor fixes

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit a465ae1)

# Conflicts:
#	client/context.go
#	simapp/simd/cmd/root.go
@mergify
Copy link
Contributor

mergify bot commented May 12, 2021

Command backport release/v0.42.x: success

Backports have been created

aaronc pushed a commit that referenced this pull request May 17, 2021
* Add env variable to cmd flags (#9040)

* first draft

* add tests in config_test package

* refactored, all tests pass, r4r

* ready for review

* add envPrefix

* Update simapp/simd/cmd/root.go

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

* fix linter issues

* minor fixes

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit a465ae1)

# Conflicts:
#	client/context.go
#	simapp/simd/cmd/root.go

* Remove line

Co-authored-by: Andrei Ivasko <cyberbono3@gmail.com>
Co-authored-by: Amaury M <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Alessio Treglia <alessio@tendermint.com>
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* first draft

* add tests in config_test package

* refactored, all tests pass, r4r

* ready for review

* add envPrefix

* Update simapp/simd/cmd/root.go

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

* fix linter issues

* minor fixes

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Environment variables are not populated to cmd flags
3 participants