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(rollup, coordinator, bridge-history-api): Allow Environment Variables to override config.json #1489

Merged
merged 13 commits into from
Aug 23, 2024

Conversation

dghelm
Copy link
Contributor

@dghelm dghelm commented Aug 20, 2024

Purpose or design rationale of this PR

The goal is to allow services to be configured using environmental variables (in addition to the config.json file). This facilitates contexts like Kubernetes, where you want Secrets managed independently from the non-sensitive config information (say, loaded from AWS Secrets).

Here, variables are prefixed with SCROLL_[SERVICE]_.

Sensitive values include:

  • SCROLL_ROLLUP_DB_CONFIG_DSN
  • SCROLL_ROLLUP_L1_CONFIG_RELAYER_CONFIG_GAS_ORACLE_SENDER_PRIVATE_KEY
  • SCROLL_ROLLUP_L2_CONFIG_RELAYER_CONFIG_GAS_ORACLE_SENDER_PRIVATE_KEY
  • SCROLL_ROLLUP_L2_CONFIG_RELAYER_CONFIG_COMMIT_SENDER_PRIVATE_KEY
  • SCROLL_ROLLUP_L2_CONFIG_RELAYER_CONFIG_FINALIZE_SENDER_PRIVATE_KEY
  • SCROLL_BRIDGE_HISTORY_DB_DSN
  • SCROLL_BRIDGE_HISTORY_REDIS_PASSWORD
  • SCROLL_COORDINATOR_AUTH_SECRET
  • SCROLL_COORDINATOR_DB_DSN

I have not run this code in the Scroll SDK stack, and only a minimal test has been run to see if it works. This at least shows the string is being used as though it came from the JSON file.

❯ export SDKROLLUP_DB_CONFIG_DSN=postgres://postgres:azerty12345@postgresql-rollup:5432/scroll?sslmode=disable
❯ ./build/bin/gas_oracle --config ./conf/config.json
ERROR[08-20|13:44:18.305] gorm                                     err message="failed to initialize database, got error failed to connect to `host=postgresql-rollup user=postgres database=scroll`: hostname resolving error (lookup postgresql-rollup on 10.255.255.254:53: read udp 10.255.255.254:54234->10.255.255.254:53: i/o timeout)"
CRIT [08-20|13:44:18.305] failed to init db connection             err="failed to connect to `host=postgresql-rollup user=postgres database=scroll`: hostname resolving error (lookup postgresql-rollup on 10.255.255.254:53: read udp 10.255.255.254:54234->10.255.255.254:53: i/o timeout)"

Edited to include additional services.
Also see:

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

  • No, this PR doesn't involve a new deployment, git tag, docker image tag
  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change
  • Yes

@dghelm dghelm requested a review from colinlyguo August 20, 2024 18:55
@georgehao
Copy link
Member

BTW, what's the rationale of scroll-sdk needing ENV ? since there are some many options for rollup_realyer and gas_oracle

@dghelm dghelm marked this pull request as ready for review August 21, 2024 12:48
@dghelm dghelm requested a review from georgehao August 21, 2024 12:48
@dghelm
Copy link
Contributor Author

dghelm commented Aug 21, 2024

Adjusted prefix to SCROLL_ROLLUP to not be SDK specific.

@colinlyguo colinlyguo added the bump-version Bump the version tag for deployment label Aug 21, 2024
@codecov-commenter
Copy link

codecov-commenter commented Aug 21, 2024

Codecov Report

Attention: Patch coverage is 2.53165% with 77 lines in your changes missing coverage. Please review.

Project coverage is 52.20%. Comparing base (7c95208) to head (643c29e).
Report is 1 commits behind head on develop.

Files Patch % Lines
common/utils/utils.go 0.00% 71 Missing ⚠️
coordinator/internal/config/config.go 25.00% 2 Missing and 1 partial ⚠️
rollup/internal/config/config.go 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1489      +/-   ##
===========================================
- Coverage    52.52%   52.20%   -0.32%     
===========================================
  Files          156      156              
  Lines        12528    12607      +79     
===========================================
+ Hits          6580     6582       +2     
- Misses        5382     5457      +75     
- Partials       566      568       +2     
Flag Coverage Δ
bridge-history-api 71.16% <ø> (ø)
common 42.98% <0.00%> (-4.37%) ⬇️
coordinator 17.41% <25.00%> (+0.01%) ⬆️
database 42.85% <ø> (ø)
rollup 57.04% <25.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dghelm dghelm changed the title feat(rollup): Allow Environment Variables to override config.json feat(rollup, coordinator, bridge-history-api): Allow Environment Variables to override config.json Aug 21, 2024
colinlyguo
colinlyguo previously approved these changes Aug 21, 2024
@dghelm dghelm requested a review from Thegaram August 22, 2024 14:41
Thegaram
Thegaram previously approved these changes Aug 22, 2024
common/utils/utils.go Show resolved Hide resolved
Co-authored-by: Péter Garamvölgyi <peter@scroll.io>
@colinlyguo colinlyguo dismissed stale reviews from Thegaram and themself via 6397274 August 22, 2024 15:56
@dghelm dghelm requested a review from Thegaram August 22, 2024 17:06
@dghelm dghelm requested a review from yiweichi August 23, 2024 01:27
Copy link
Member

@yiweichi yiweichi left a comment

Choose a reason for hiding this comment

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

LGTM

@yiweichi yiweichi merged commit e09b98f into develop Aug 23, 2024
17 checks passed
@yiweichi yiweichi deleted the feat-envvar-for-rollup branch August 23, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump-version Bump the version tag for deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants