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

Cardano-db-sync module #59

Merged
merged 24 commits into from
May 16, 2024
Merged

Cardano-db-sync module #59

merged 24 commits into from
May 16, 2024

Conversation

zmrocze
Copy link
Contributor

@zmrocze zmrocze commented Apr 9, 2024

solves issue #58.

Cardano-db-sync module. With cardano-db-sync.postgres.enable also runs postgres.
Single test that awaits sync progress to start.

@zmrocze zmrocze marked this pull request as draft April 9, 2024 15:42
@zmrocze zmrocze requested review from brainrake, avnik and aciceri April 30, 2024 12:37
@zmrocze zmrocze marked this pull request as ready for review April 30, 2024 12:38
@zmrocze
Copy link
Contributor Author

zmrocze commented Apr 30, 2024

eh, attic input fails to build with these nixpkgs. but still id say go ahead and review

@zmrocze
Copy link
Contributor Author

zmrocze commented Apr 30, 2024

our nixos tests run forever in CI. Locally it's maybe 25 secs for me. But previously the db-sync failed after reaching 100s timeout (which it reached after many minutes (?))

Copy link
Member

@aciceri aciceri left a comment

Choose a reason for hiding this comment

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

I cannot run the test locally using
nix run .#apps.x86_64-linux.vmTests-cardano-db-sync
I get the following error:

nix run .#apps.x86_64-linux.vmTests-cardano-db-sync

Check also #69

Anyway the test seems good! 💪 You said it works locally, right? Any idea why it doesn't work on CI? You increased the timeout and decreased the target percentage, both reasonable actions.

flake.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
}
{
assertion = (! cfg.postgres.enable) || (cfg.database.name == cfg.database.user);
message = "When postgres is enabled, we use the ensureDBOwnership option which expects the user name to match db name.";
Copy link
Contributor

Choose a reason for hiding this comment

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

do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

          ensureUsers = [
            {
              name = "${cfg.database.name}";
              ensureDBOwnership = true;
            }
          ];

this means that cfg.database.name has access to cfg.database.name database, so if database.name is different, then the user needs to overwrite these permissions himself.
But I should make it a warning I now think.

Copy link
Contributor

Choose a reason for hiding this comment

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

added assertion

modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/default.nix Outdated Show resolved Hide resolved
tests/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
modules/cardano-db-sync.nix Outdated Show resolved Hide resolved
@zmrocze
Copy link
Contributor Author

zmrocze commented May 14, 2024

@brainrake i think it's fine now, would you check once more?
I simply removed the marked redundant options and the redundent postgres authentication setting (relic of some fix while experimenting).

Irrelevant note on the side:

Before I was under the impression that you're supposed to abstract away the underlying module (here services.cardano-db-sync) with your module (cardano.db-sync) - and make the user not touch the underlying module. I see it's differently with nixos modules, but it does make some sense, I mean it's more direct and doesn't repeat and in the end the module abstraction is always going to be leaky. On the downside the user needs to not only read the option docs but also the source including imports.

@brainrake
Copy link
Contributor

Thanks @zmrocze , that makes sense. I think you solved one part by mentoning services.cardano-db-sync in the cardano.db.sync.enable documentation. We can make it more explicit, or add a link. Another part is rendering the documentation for the services.* modules: #71 .

Let me explain. There were upstream modules for cardano-node and cardano-db-sync. Wrapping everything leads to a large amount of extra code with no apparent advantage except having all the options in the same namespace. So we went the opposite way, not wrapping anything, just adding the minimum options to support default use cases (sometimes just enable), and setting options to coordinate between modules. You can set up the default use cases with the very few options under cardano.*. If you need more configurability, you can use the modules.
Another advantage is that modules under services.* without any dependencies by just importing the file. They can be easily contributed upstream or to nixpkgs.

@brainrake brainrake merged commit 01692b5 into main May 16, 2024
2 checks passed
@brainrake brainrake deleted the karol/db-sync branch May 16, 2024 21:12
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.

3 participants