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

Allow to run docker with DISABLE_LEDGER (#1430) #1436

Merged
merged 2 commits into from
Jun 28, 2023

Conversation

sgillespie
Copy link
Contributor

@sgillespie sgillespie commented Jun 14, 2023

Description

Fixes #1430. This change allows users to pass an environment variable DISABLE_LEDGER to the docker container (and the NixOS module). If this flag is non-empty, it will pass --disable-ledger and omit the --state-dir ... option.

Note that even when services.cardano-db-sync.disableLedger is true, stateDir is still defaulted and required, because we write pgpass there.

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu (which can be run with scripts/fourmolize.sh
  • Self-reviewed the diff

Migrations

  • The pr causes a breaking change of type a,b or c
  • If there is a breaking change, the pr includes a database migration and/or a fix process for old values, so that upgrade is possible
  • Resyncing and running the migrations provided will result in the same database semantically

If there is a breaking change, especially a big one, please add a justification here. Please elaborate
more what the migration achieves, what it cannot achieve or why a migration is not possible.

@sgillespie sgillespie force-pushed the feature/disable-ledger-docker branch from b0ce3a2 to 2f1b8e8 Compare June 14, 2023 20:16
nix/docker.nix Outdated
exec $DBSYNC \
--schema-dir ${../schema} \
--state-dir ${scripts.mainnet.db-sync.passthru.service.stateDir} $@
if [[ -n "$DISABLE_LEDGER" ]]; then

Choose a reason for hiding this comment

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

I understand you just want to check whether DISABLE_LEDGER is set or not but, given you write DISABLE_LEDGER=true in the documentation above, I am worried someone will do DISABLE_LEDGER=false 😂.
Maybe checking for an explicit value of DISABLE_LEDGER (e.g. "true" or 1) could be a safer option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. After looking around it, it looks like there's an existing pattern (RECREATE_DB=Y), so I think it would make sense to follow that pattern. Source: https://github.com/input-output-hk/cardano-db-sync/blob/master/scripts/postgresql-setup.sh#L311

@@ -177,6 +185,20 @@ in {
(if (cfg.environment.nodeConfig.RequiresNetworkMagic == "RequiresNoMagic" )
then "--mainnet"
else "--testnet-magic $(jq '.networkMagic' ${cfg.environment.nodeConfig.ShelleyGenesisFile})");

Choose a reason for hiding this comment

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

Just a thought, could we get the networkMagic in nix rather than going through jq?

@sgillespie sgillespie force-pushed the feature/disable-ledger-docker branch from f9dcec0 to ee5f8f1 Compare June 15, 2023 17:49
@sgillespie sgillespie force-pushed the feature/disable-ledger-docker branch from ee5f8f1 to a2b36e6 Compare June 15, 2023 18:01
@sgillespie sgillespie marked this pull request as ready for review June 15, 2023 18:07
@sgillespie sgillespie requested review from a team as code owners June 15, 2023 18:07
@kderme kderme merged commit 1c01b34 into IntersectMBO:master Jun 28, 2023
36 of 39 checks passed
@sgillespie sgillespie deleted the feature/disable-ledger-docker branch August 29, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to run docker without --state-dir
4 participants