hercules-ci / hercules-ci-effects

Expressions to change the world (just a tiny bit)
Apache License 2.0
16 stars 10 forks source link

Add populate-cache-effect #163

Closed zmrocze closed 8 months ago

zmrocze commented 10 months ago

Motivation

We want to push some paths to either the cachix or attic cache, additionaly to the cache configured for herculesCI. We discussed it recently on mlabs slack. If you guys would be interested here's a module doing that to be used like below:

populate-cache-effect = {
      enable = true;
      caches = {
        mlabs-attic = {
          type = "attic";
          secretName = "cardano-nix-attic-token";
          branches = ["master"];
          packages = [test-drv];
        };
        mlabs-cachix = {
          type = "cachix";
          secretName = "karol-cachix-token";
          branches = ["master" "push-to-attic"];
          packages = [test-drv];
        };
      };
    };

it's a flake-parts module producing effects under herculesCI.onPush.default.outputs.effects.populate-cache-effect option.

Questions

It works. But I don't understand the structure of this repository, so I'd like to ask first how to incorporate that into the codebase:

Maintainer checklist

roberth commented 10 months ago

You're using push and populate interchangeably, but push seems to be the established term for what we're trying to achieve. Perhaps populate could be renamed to push, or does that lose something subtle I don't catch on to?

roberth commented 10 months ago

There's a potential "trap" (affectionately) because the cache name is not necessarily unique between cache providers / instances. This could be resolved by making the name overridable, but that requires a little bit of creative thought on the user's end, which is not ideal. Maybe the option structure could be changed to make this situation easier to deal with. Changing the top level structure to a list might achieve this, but at the cost of overridability, for what it's worth. I hope there's another way that I just haven't thought of yet.

aciceri commented 10 months ago

What if we have something like:

caches = {
  "attic:foo" = {
    # ...
  };
  "cachix:foo" = {
    # ...
  };
}

Where the attrset name is parsed and used to set the cache's name and type options' defaults.

roberth commented 9 months ago
"attic:foo"

I like this. It's basically the "store URI" concept in Nix, but then extended to ones Nix does not actually have, which kind of begs to support the ones that Nix supports natively, such as S3. nix copy would be the backend for those. It can be tested with minio or even the ssh: store URIs. I see attic also comes with a NixOS module, do it'd be good to have an effectVMTest for it, but I see that we only have those for individual effects at this point, not for whole flake-parts modules.

Ideally this effect would be usable outside a flake-parts context as well, but I know that it's currently non-trivial how to get good reuse between flake-parts modules and modularEffect modules (ie module system applied to mkEffect). I can't do it right now, so I feel like it'd probably best for me to go in later and do some refactoring, so let's not block this PR on refactoring and testing.

zmrocze commented 9 months ago

There's a potential "trap" (affectionately) because the cache name is not necessarily unique between cache providers / instances.

I dont think there's problem. The name option, inherited from the attribute name, is just the name for the effect. The cache name (as in cachix push <cache-name>) is provided in the secret.

zmrocze commented 9 months ago

Thanks for review @aciceri and @roberth. I'll shortly apply the suggestions, only one think I don't know is where to export this flake-parts module from the flake then?

I didn't like that it's flake-part module specifically and not modularEffect module, but I also didn't know how to do it better, so I'd leave it at that if you allow.

zmrocze commented 9 months ago

Moved to https://github.com/mlabs-haskell/hercules-ci-effects/pull/1 to allow edits

zmrocze commented 8 months ago

moved to #165 to originate from the same branch as the demo project

brainrake commented 8 months ago

Regarding passing the type option in a shorter way: https://github.com/hercules-ci/hercules-ci-effects/pull/165/files#r1472201090

brainrake commented 8 months ago

I have reviewed all the above comments and moved everything unresolved to the new issue #165.