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

fix: bind values from env variables to flags #8337

Merged
merged 1 commit into from
Jan 29, 2021
Merged

fix: bind values from env variables to flags #8337

merged 1 commit into from
Jan 29, 2021

Conversation

troian
Copy link
Contributor

@troian troian commented Jan 14, 2021 •
edited
Loading

allows clients access values in env variables

Signed-off-by: Artur Troian troian.ap@gmail.com

Description

allows clients access values in env variables

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

@alessio alessio requested review from fdymylja, jgimeno, sahith-narahari and robert-zaremba and removed request for alessio and robert-zaremba January 14, 2021 16:33
robert-zaremba
robert-zaremba requested changes Jan 15, 2021
View reviewed changes
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Let's update a documentation (/docs) as well:

  • note how the ENV variable name is created
  • As far as I can see, we support an ENV variable for every flag - so let's describe it in docs as well.

@alessio
Copy link
Contributor

alessio commented Jan 15, 2021

I agree with @robert-zaremba, it'd be nice to have the documentation updated. Still, I couldn't find out a good entry point in the docs for this information. @robert-zaremba any suggestion?

@troian
Copy link
Contributor Author

troian commented Jan 16, 2021

@robert-zaremba @alessio sure, i'll doc changes, just let me know what section fits best

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Jan 18, 2021 •
edited
Loading

@troian I think it should go under ### Improvements for changelog.

@robert-zaremba
Copy link
Collaborator

As for docs, @amaurymartiny is moving CLI docs to a new location: https://github.com/cosmos/cosmos-sdk/pull/8294/files#diff-2bc8f9d695cf7d1f01a06afbdc287c4d5d84345e24299ca83e58c67a1ed0d319, so let's wait until his PR get merged.

@amaury1093
Copy link
Contributor

The PR Robert mentioned got merged. @troian you can put it in docs/core/cli.md.

@troian troian requested a review from robert-zaremba January 24, 2021 21:40
amaury1093
amaury1093 approved these changes Jan 25, 2021
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK

Can get in once tests are fixed

@amaury1093 amaury1093 added A:automerge Automatically merge PR once all prerequisites pass. backport/0.40.x (Stargate) labels Jan 25, 2021
@troian
Copy link
Contributor Author

troian commented Jan 25, 2021

does not seem these fails related to the PR. Panic happens before bind process happens. is there a issue for it?

@amaury1093
Copy link
Contributor

does not seem these fails related to the PR.

Could you double-check about this? This test is not failing on master.

@troian
fix: bind values from env variables to flags
626ac7b 
allows clients access values in env variables

Signed-off-by: Artur Troian <troian.ap@gmail.com>
@mergify mergify bot merged commit c252a60 into master Jan 29, 2021
@mergify mergify bot deleted the fix-env branch January 29, 2021 01:58
@amaury1093
Copy link
Contributor

nice, thanks! @troian would you be willing to create a backport PR to the release/v0.41.x branch, by cherry-picking this commit?

@troian
Copy link
Contributor Author

troian commented Jan 29, 2021

sure

@troian
Copy link
Contributor Author

troian commented Jan 29, 2021

@amaurymartiny release/v0.41.x does not have docs/core/cli.md in

@alessio alessio mentioned this pull request Feb 10, 2021
Merged
@clevinson clevinson mentioned this pull request Feb 10, 2021
Closed
23 tasks
amaury1093 pushed a commit that referenced this pull request Feb 16, 2021
@troian @amaury1093
fix: bind values from env variables to flags (#8337)
95b67c7 
allows clients access values in env variables

Signed-off-by: Artur Troian <troian.ap@gmail.com>
clevinson pushed a commit that referenced this pull request Feb 17, 2021
@troian
fix: bind values from env variables to flags (#8337) (#8488)
963de42 
allows clients access values in env variables

Signed-off-by: Artur Troian <troian.ap@gmail.com>
@clevinson clevinson mentioned this pull request Feb 24, 2021
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alessio alessio alessio approved these changes

@robert-zaremba robert-zaremba robert-zaremba approved these changes

@amaury1093 amaury1093 amaury1093 approved these changes

@fdymylja fdymylja fdymylja approved these changes

@aaronc aaronc Awaiting requested review from aaronc

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@jgimeno jgimeno Awaiting requested review from jgimeno

@sahith-narahari sahith-narahari Awaiting requested review from sahith-narahari

Assignees
No one assigned
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Environment variables are not populated to cmd flags
5 participants
@troian @alessio @robert-zaremba @amaury1093 @fdymylja