graphprotocol / graph-node

Graph Node indexes data from blockchains such as Ethereum and serves it over GraphQL
https://thegraph.com
Apache License 2.0
2.9k stars 968 forks source link

[Feature] Replace all custom env. var. expansion logic for configuration files with `a8m/envsubst` #4678

Open neysofu opened 1 year ago

neysofu commented 1 year ago

Description

graph-node supports env. var. expansion in certain sections of the configuration files, and we also ship envsubst in the official Docker image for more complex use cases. I'd argue it makes most sense to drop our custom env. var. expansion logic and automatically invoke envsubst in the start script. This benefits us in three ways:

  1. Less code – yay, always a win.
  2. We enable env. var. expansion in all sections of the configuration files, as opposed to just some of them.
  3. We get support for default values and other advanced features of envsubst, like custom evaluation expressions.

Are you aware of any blockers that must be resolved before implementing this feature? If so, which? Link to any relevant GitHub issues.

No response

Some information to help us out

maoueh commented 1 year ago

I'd like to keep env expansion built in at least for those already existing. I used that feature for development in different places. Yes I could switch to wrap my graph-node invocation with envsubst, but I still raise the point that it's used today.

Some from Semiotic looked at having env expansion deeper in the deserialization engine to that the full config could use env expansion, not sure what have been the state/conclusion of that.

neysofu commented 1 year ago

Thanks for the feedback @maoueh. The idea of replacing our expansion logic with envsubst made sense to me because it would be a much better alternative to what we have right now, i.e. a very inconsistent and undocumented templating engine. A custom tool would do a much better job.

That said, Semiotic's plan of running env. var. expansion on the entire configuration file is good enough imo and we get to keep it inside graph-node. There's also another factor that I didn't consider when I opened this issue – introducing a separate step to invoke envsubst introduces room for error when dealing with version control. graph-node configuration files are full of secrets, storing the expanded version on file would make it way too easy to accidentally commit them to git.

I'm going to keep this issue open to track future ideas around env. var. expansion, but I've reconsidered envsubst as an external tool and I don't think it's the best replacement for what we have.

leoyvens commented 1 year ago

The Figment crate enables layered configuration. Looks very cool really, it's not exactly envsubst but it's an interesting approach to overriding from the environment. The key to be substituted can be entirely omitted in the toml. The env var must have a specified prefix and path format. And it can support other use cases such as overriding TOML with another TOML or a JSON.

github-actions[bot] commented 7 months ago

Looks like this issue has been open for 6 months with no activity. Is it still relevant? If not, please remember to close it.