Add populate-cache-effect by zmrocze · Pull Request #163 · hercules-ci/hercules-ci-effects

zmrocze · 2023-12-02T21:32:10Z

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:

where/how to export this
if it's fine the way it is with default attic-client-pkg cachix-pkg

Maintainer checklist

Documentation (function reference docs, setup guide, option reference docs)
Has tests (VM test, free account, and/or test instructions)
Is the corresponding module up to date?

aciceri · 2023-12-04T11:43:45Z

+
+  options = {
+    populate-cache-effect = {
+      enable = lib.mkOption {


Consider lib.mkEnableOption

I prefer to make a more the description of enable longer than what mkEnableOption does, so I'm always happy to forget its syntax and just write it out.

I think the description on the line below is a good place to explain the relevance and some subtleties of this effect, that can't be captured in the other options. E.g.

description = '' Enables an effect that pushes certain outputs to a different binary cache. Hercules CI normally pushes everything to the cache(s) configured on the agent. This effect supplements that behavior by letting you push a subset of those to a different cache. '';

Also:

Note that it only pushes the output closure, and not the closures of build dependencies used during the build stage of the CI job. (Unless those closures happen to also be part of the output or "runtime" closure)

Hasn't mkEnableOption an optional description argument?

I discussed this with someone at some point, but I don't think anyone followed through on it. It'd be nice.

Resolved: leave it as is.

aciceri · 2023-12-04T11:44:05Z

+        description = "Enables HerculesCI effects populating some external cache.";
+      };
+      attic-client-pkg = lib.mkOption {
+        type = lib.types.package;


Consider lib.mkPackageOption

used it but reverted in the end to keep consistent with attic-pkg option

Resolved: leave it as is.

aciceri · 2023-12-04T11:48:02Z

+              secretName = lib.mkOption {
+                type = lib.types.str;
+                description = ''
+                  Name of the HerculesCI secret. See [HerculesCI docs](https://docs.hercules-ci.com/hercules-ci-agent/secrets-json).
+                  The secrets "data" field should contain given data:
+
+                  ```
+                    "data": {
+                      "name": "my-cache-name",
+                      "token": "ey536428341723812",
+                      "endpoint": "https://my-cache-name.com"
+                    }
+                  ```
+
+                  The "endpoint" field is needed for Attic cache. With Cachix cache the "endpoint" field is not read and can be absent.
+                '';
+              };


I would add a default for this option (e.g. "hci-cache-${name}-settings") in order to decrease the amount of needed boilerplate.

Fewer defaults means it may be easier to grep for usages of a secret.
Ideally the underlying search problem could be solved with tooling, so I'm on the fence about this issue.

I hadn't even asked myself this search problem, I defer to your judgement :)

I would leave without a default as otherwise there is still strong coupling to an outside piece of configuration only it's hidden away, which is worse. I find myself spending a lot of time figuring out these hidden globals in various systems.

Resolved: leave it as is.

aciceri · 2023-12-04T11:52:08Z

+              branches = lib.mkOption {
+                type = with lib.types; listOf str;
+                description = ''
+                  Branches on which we'd like to execute the effect.
+                '';
+              };


How should I set this option if I wanted to push to the caches for all the branches? Moreover I believe that this behavior should be the default one.

@roberth effects aren't executed on forks, are they? Just to be sure, we don't want that people are able to push outputs to our caches simply by forking our repos.

Correct, onPush only activates for the "local" repo - ie the one that had Hercules CI installed on it via GitHub, in the organization for which the agent is configured to run.

Any exceptions to this rule will be opt-in, one way or another, because it's important for security. After all, CI can be described as pure "remote code execution".

@aciceri Right. Let's use nullOr (listOf str) for that?

made it nil by default as well

Resolved: fixed.

aciceri · 2023-12-04T12:02:35Z

+  inputs.attic = {
+    url = "github:zhaofengli/attic";
+    inputs = {
+      nixpkgs.follows = "nixpkgs";
+    };
+  };


Unfortunately attic is not in nixpkgs (not even the client). Personally I don't like forcing an extra input to all the hercules-ci-effect consumers (inputs are not lazy). Moreover we don't get caching for it.

Perhaps we should consider adding it to nixpkgs (should be trivial, I'm referring only to the client derivation) or leaving to hercules-ci-effects consumers the burden to fetch that derivation and set it here.

Let's see what @roberth says

It is unfortunate that transitive inputs are so "in your face". I intend to change this in Nix, but with limited resources.
See NixOS/nix#7730

Until then I agree it's perhaps best not to have a flake-level dependency like this.
Ideally it could be added to Nixpkgs. A cheap way to "solve" the problem is by making it the caller's problem:

default = pkgs.attic or (throw "<option>: It seems that attic hasn't been packaged in Nixpkgs (yet?). Please check <nixpkgs packaging request issue> or set <option> manually.");

If that proves infeasible, we could consider doing something custom.

Nixpkgs package request: Attic NixOS/nixpkgs#232532

Resolved: fixed (made it caller's problem).

roberth · 2023-12-04T13:16:30Z

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 · 2023-12-04T13:21:27Z

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 · 2023-12-04T13:45:30Z

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 · 2023-12-05T16:15:57Z

"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 · 2023-12-07T17:16:16Z

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 · 2023-12-07T17:25:19Z

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.

avnik · 2023-12-09T06:49:52Z

@@ -0,0 +1,179 @@
+{inputs, withSystem, ...}:
+let
+  attic-client = inputs.attic.packages."x86_64-linux".attic-client;


Why here is hardcoded x86_64-linux system, shouldn't we use herculesCI.ciSystems here?

Maybe, because I dont understand how system plays with effects.

@roberth Could you help? Am I free to use any system I find convenient for effect definition? What happens if I mention darwin derivation paths in my linux (linux in a sense that it uses withSystem "linux" ({hci-effects}: ...)) effect?
Specifically here, do I need separate effects for pushing paths for different systems to cache?

Unresolved. Moved to: https://github.com/hercules-ci/hercules-ci-effects/pull/165/files#r1472198232

zmrocze · 2023-12-18T17:14:07Z

Moved to mlabs-haskell#1 to allow edits

zmrocze · 2024-01-11T16:47:45Z

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

brainrake · 2024-01-31T01:35:02Z

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

brainrake · 2024-01-31T01:35:45Z

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

Add populate-cache-effect

e808278

zmrocze requested a review from roberth as a code owner December 2, 2023 21:32

aciceri reviewed Dec 4, 2023

View reviewed changes

zmrocze added 2 commits December 7, 2023 19:27

Remove attic flake input & Rename to push & small fixes

2bb0b93

Fix branches option

ffdf649

avnik reviewed Dec 9, 2023

View reviewed changes

aciceri mentioned this pull request Dec 15, 2023

attic-client: init at unstable-2024-02-08 NixOS/nixpkgs#274481

Merged

13 tasks

zmrocze added 2 commits December 18, 2023 18:03

Rename

bfc022d

Export push-cache effect

013989c

zmrocze added 5 commits December 18, 2023 18:23

Move export

986c1e0

Format

67c756a

Fix

e12d297

Fix path

285d7c7

Rename and unformat

3fa860b

zmrocze closed this Jan 11, 2024

Conversation

zmrocze commented Dec 2, 2023

Motivation

Questions

Maintainer checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciceri Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberth commented Dec 4, 2023

Uh oh!

roberth commented Dec 4, 2023

Uh oh!

aciceri commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberth commented Dec 5, 2023

Uh oh!

zmrocze commented Dec 7, 2023

Uh oh!

zmrocze commented Dec 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zmrocze commented Dec 18, 2023

Uh oh!

zmrocze commented Jan 11, 2024

Uh oh!

brainrake commented Jan 31, 2024

aciceri Dec 4, 2023 •

edited

Loading

aciceri commented Dec 4, 2023 •

edited

Loading