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

feat!: Type definition specification with env vars and JSON files #399

Merged
merged 3 commits into from
Jan 20, 2021

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Jan 19, 2021

closes: #387

changes

BREAKING CHANGE remove /config/types.json; Functionality replaced with SAS_SUBSTRATE_TYPES.

Additionally the following environment variables are added:

  • SAS_SUBSTRATE_TYPES_BUNDLE
  • SAS_SUBSTRATE_TYPES_CHAIN
  • SAS_SUBSTRATE_TYPES_SPEC

motivation

This will allow users to update chain types when there is a delay for type updates in the apps-config package supply chain or if a chain's type definitions simply do not exist in the apps-config.

BREAKING CHANGE remove /config/types.json; Replaced with SAS_SUBSTRATE_TYPES.
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

This seems to get the job done.

Do you think it'd be good long term to switch to using a proper config file, rather than using env variables? So we'd have substrate-api-sidecar --config=my-config.[json|toml|yaml] and have all the type overrides and other configuration as keys in there?

README.md Outdated Show resolved Hide resolved
src/main.ts Show resolved Hide resolved
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

LGTM. I like how it gives the end user more flexibility.

@emostov
Copy link
Contributor Author

emostov commented Jan 19, 2021

Do you think it'd be good long term to switch to using a proper config file, rather than using env variables? So we'd have substrate-api-sidecar --config=my-config.[json|toml|yaml] and have all the type overrides and other configuration as keys in there?

@dvdplm Right now the equivalent would be putting all your env vars in a .env file, like .env.polkadot. Then the user would start up sidecar with NODE_ENV=polkadot yarn start. Do you think a yml type config file would be an improvement to this?

@dvdplm
Copy link
Contributor

dvdplm commented Jan 19, 2021

@dvdplm Right now the equivalent would be putting all your env vars in a .env file, like .env.polkadot. Then the user would start up sidecar with NODE_ENV=polkadot yarn start. Do you think a yml type config file would be an improvement to this?

I do have a distaste for ENV vars, but my opinion on this is definitely a minority. My gut reaction to .env files is "well, why not use a proper config file then?". I think env vars are opaque to users, fiddly to set, even fiddlier to unset, tricky to document properly, no namespaces so they grow really long and fugly.
I think using a good config file format, like TOML (and to a lesser extent YAML) or even .ini has a psychological impact on developers to document and test their configs.

But yeah, ENV vars are really popular with many smart people so I tend to keep my opinions to myself. They're good from the devops POV for sure, and has good tooling on that end.

When I wrote my first comment what I had in mind was the ability to merge configs from several places: a decent set of defaults from substrate, refined further from polkadot and finally overrideable by users for their own chains. Such a hierarchy of config values is tractable using config files in a sane format; with ENV vars not so much?

@emostov
Copy link
Contributor Author

emostov commented Jan 19, 2021

@dvdplm Right now the equivalent would be putting all your env vars in a .env file, like .env.polkadot. Then the user would start up sidecar with NODE_ENV=polkadot yarn start. Do you think a yml type config file would be an improvement to this?

I do have a distaste for ENV vars, but my opinion on this is definitely a minority. My gut reaction to .env files is "well, why not use a proper config file then?". I think env vars are opaque to users, fiddly to set, even fiddlier to unset, tricky to document properly, no namespaces so they grow really long and fugly.
I think using a good config file format, like TOML (and to a lesser extent YAML) or even .ini has a psychological impact on developers to document and test their configs.

But yeah, ENV vars are really popular with many smart people so I tend to keep my opinions to myself. They're good from the devops POV for sure, and has good tooling on that end.

When I wrote my first comment what I had in mind was the ability to merge configs from several places: a decent set of defaults from substrate, refined further from polkadot and finally overrideable by users for their own chains. Such a hierarchy of config values is tractable using config files in a sane format; with ENV vars not so much?

I really have minimal opinion on this, just what ever works for the most people. Happy to explore/implement different solutions in the future - I just would need a clear direction

@emostov emostov merged commit 8c621b0 into master Jan 20, 2021
@emostov emostov deleted the zeke-types-from-file branch January 20, 2021 01:05
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B2 - Breaks API Breaking changes to the API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can I configure custom types via environment variables?
3 participants