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

Environment variables are not populated to cmd flags #8179

Closed
4 tasks
clevinson opened this issue Dec 16, 2020 · 12 comments · Fixed by #8337 or #9040
Closed
4 tasks

Environment variables are not populated to cmd flags #8179

clevinson opened this issue Dec 16, 2020 · 12 comments · Fixed by #8337 or #9040
Assignees
Milestone

Comments

@clevinson
Copy link
Contributor

Description

Certain client cmd flags historically have been able to be set via environment variables, instead of explicitly always having to set the flag (e.g. CHAIN_ID, FROM, $SIMD_HOME etc.).

This is not currently supported in Stargate and should be.

@anilcse @alexanderbez I've attempted to find documentation of exactly which flags are expected to be supported with a corresponding env var, but can't find clear evidence of the prior behavior. Feel free to add to this description or reply specific flags that should be supported.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@clevinson clevinson added this to the v0.40.1 milestone Dec 16, 2020
@anilcse
Copy link
Collaborator

anilcse commented Dec 16, 2020

@clevinson previously we had prefix'd ENV variables => flags mapping. So we shouldn't focus on adding specific flags but fix the behaviour as before. Ex: https://github.com/cosmos/gaia/blob/v2.0.14/cmd/gaiacli/main.go#L73-L74 here we configure the prefix and if we need --chain-id flag, it can be set from GA_CHAIN_ID env variable.

From docs: Environment variables can't have dashes in them, so bind them to their equivalent, keys with underscores, e.g. --favorite-color to STING_FAVORITE_COLOR

@anilcse
Copy link
Collaborator

anilcse commented Dec 16, 2020

@troian from Akash has a hacky fix

We had issues with values from environment variables not populated to flags, especially on client side. for example if I set AKASH_NODE= client never picks it. it became really painful for us, especially during testing edgenet.
we had a fix to our codebase (see this commit: akash-network/node@d2ce9b1), which looks more like a workaround, but allows us to use environment variables.
there are two issues we have discovered.
wrong initialization sequence in PersistentPreRunE . client.SetCmdClientContextHandler was called before server.InterceptConfigsPreRunHandler which does actual work of configuring viper and binding pflags. thats why client never got values from env vars. was it just typo or there that sequence must be followed?
server.InterceptConfigsPreRunHandler still has some issue with binding pflags to viper thats why we did dirty hack with bindFlags

@alexanderbez
Copy link
Contributor

No, the order does not matter.

@troian
Copy link
Contributor

troian commented Dec 17, 2020

@alexanderbez is it comment to our hack?

@alexanderbez
Copy link
Contributor

Yes :)

@clevinson
Copy link
Contributor Author

@alexanderbez Can you let me know if you're working on this? If not, we'll do some triaging and get someone from our team on it.

@alexanderbez
Copy link
Contributor

It's not assigned to me and I'm not working on it, nor do I plan to.

@amaury1093
Copy link
Contributor

This comment #8132 (comment) is relevant here, correct @alexanderbez ?

@alexanderbez
Copy link
Contributor

Yes @amaurymartiny. I really suggest we refactor the entire thing.

@clevinson
Copy link
Contributor Author

Reopening as the fix in #8337 only resolved the issue when starting the server (e.g. gaiad start) and not when running CLI commands to interact witn a running node (gaiad tx..., gaiad query...) or any of the gaiad keys... subcommands.

@clevinson
Copy link
Contributor Author

So from @anilcse it seems that the problem with the current implementation has more to do with simd not picking up dynamic changes in hte env variable.

@troian
Copy link
Contributor

troian commented Mar 5, 2021

@clevinson there is a one more thing to be changed into root PreRun hook in order to get all things working

PersistentPreRunE: func(cmd *cobra.Command, _ []string) error {
			if err := server.InterceptConfigsPreRunHandler(cmd); err != nil {
				return err
			}

			ctx := server.GetServerContextFromCmd(cmd)

			return client.SetCmdClientContextHandler(initClientCtx, cmd)
		},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment