Skip to content

Add push-cache-effect#165

Open
zmrocze wants to merge 12 commits into
hercules-ci:masterfrom
mlabs-haskell:push-cache-effect
Open

Add push-cache-effect#165
zmrocze wants to merge 12 commits into
hercules-ci:masterfrom
mlabs-haskell:push-cache-effect

Conversation

@zmrocze
Copy link
Copy Markdown

@zmrocze zmrocze commented Jan 11, 2024

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?

@zmrocze zmrocze requested a review from roberth as a code owner January 11, 2024 16:46
@zmrocze zmrocze mentioned this pull request Jan 11, 2024
3 tasks
@zmrocze
Copy link
Copy Markdown
Author

zmrocze commented Jan 11, 2024

Hi @roberth,

it works as witnessed on this demo repository.

I've moved the pr again, sorry. That is the previous, there was a discussion, I tried to act on all points and believe they're fixed. Go ahead and review if you can please!

packages = lib.mkOption {
type = with lib.types; listOf package;
description = "List of packages to push to the cache.";
example = "[ pkgs.hello ]";
Copy link
Copy Markdown

@brainrake brainrake Jan 30, 2024

Choose a reason for hiding this comment

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

As a default, we could use flakeToOutputs from hercules-ci-agent to match the behavior of the default build
https://github.com/hercules-ci/hercules-ci-agent/blob/0e9e67bb7d41ea2bbb13982887ff3b9c511a2aba/hercules-ci-agent/data/default-herculesCI-for-flake.nix#L48
or combine herculesCI.onPush.*.outputs to match the hercules ci build phase
https://docs.hercules-ci.com/hercules-ci-agent/evaluation#attribute-herculesCI.onPush-outputs

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Interesting! Didn't know flakeToOutputs

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would involve writing a attribute tree traversal to match the agent behavior, with the right recuresIntoAttrs behavior, rejecting things with _type, etc.
That could be done, but I wouldn't encode into this module the assumption that every output should be pushed.

Note also that this effect can not replicate the whole agent behavior, such as pushing all the right build dependencies.
This means that it's not a replacement for the cache that's used by the agents transparently.
More significantly, it also means that this effect is not 100% suitable as a build cache or "development cache", so I doubt that e.g. the checks or the devShell dependencies should be pushed by it. A better use case for this effect is as a "release cache" - for only the end products, such as packages, or perhaps also paths for deployment, such as nixosConfigurations or the inputs of effects (if flake consumers use the same effect functions).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved: we leave it unspecified and the responsibility of the caller since replicating default builder behavior is infeasible.

@@ -0,0 +1,160 @@
{ lib, withSystem, config, ... }:
let pkgs-x86_64-linux = withSystem "x86_64-linux" ({ pkgs, ... }: pkgs);
Copy link
Copy Markdown

@brainrake brainrake Jan 31, 2024

Choose a reason for hiding this comment

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

We should support other systems.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would see what happens if we try to push non x86_64-linux derivations using an x86_64-linux effect before.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Those will be scheduled during the build phase of the job, and substituted into the agent's store before the effect is launched.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested: pushing derivations built on other systems to attic works.

imports = [ ../../flake-module.nix ];

options = {
push-cache-effect = {
Copy link
Copy Markdown

@brainrake brainrake Jan 31, 2024

Choose a reason for hiding this comment

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

We can have a cachix-push-effect and attic-push-effect to mirror cachix-deploy-effect instead of the type option. That could resolve passing the type in an unobtrusive way.

Comment thread effects/push-cache/default.nix Outdated
secretsMap = { token-file = "${cacheOptions.secretName}"; };
userSetupScript = ''
attic login \
server-name \
Copy link
Copy Markdown

@brainrake brainrake Jan 31, 2024

Choose a reason for hiding this comment

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

This makes for a very confusing error message. We should use server at least.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved: using default.

'';
type = lib.types.attrsOf (lib.types.submodule ({ name, ... }: {
options = {
name = lib.mkOption {
Copy link
Copy Markdown

@brainrake brainrake Jan 31, 2024

Choose a reason for hiding this comment

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

The default should be be cachix|attic-push-effect or cachix|attic-push-${attrname}-effect to mirror cachix-deploy-effect.


```
"data": {
"name": "my-cache-name",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should name and endpoint be in the secret? I think the attribute name can be used as name, and the endpoint doesn't need to be secret, it can be an option. But it's convenient to manage together with the token.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think both are acceptable solutions.

@brainrake
Copy link
Copy Markdown

needs documentation

@brainrake
Copy link
Copy Markdown

i would separate the two effects since the abstraction is too complicated, there are significant differences, and there is still repetition.

@brainrake
Copy link
Copy Markdown

I'd also add mkCachixPushEffect and mkAtticPushEffect functions.

@zmrocze
Copy link
Copy Markdown
Author

zmrocze commented May 6, 2024

@brainrake what if instead type would take a list, i.e.: types = ["attic" "cachix"]?
How it is currently I agree that the effect should be split into two effects.

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.

4 participants