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.

Sign up for GitHub

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

borgmatic: Allow top-level extra options #3793

Closed
wants to merge 2 commits into from
Closed

borgmatic: Allow top-level extra options #3793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11a8b48
borgmatic: Allow top-level extra options
DamienCassou Mar 20, 2023
93982b3
WIP
DamienCassou Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 31 additions & 15 deletions modules/programs/borgmatic.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ let
};

configModule = types.submodule ({ config, ... }: {
config.location.extraConfig.exclude_from =
config.extraConfig.location.exclude_from =
mkIf config.location.excludeHomeManagerSymlinks
(mkAfter [ (toString hmExcludeFile) ]);
options = {
Expand Down Expand Up @@ -73,8 +73,6 @@ let
default = false;
example = true;
};

extraConfig = extraConfigOption;
};

storage = {
Expand All @@ -84,7 +82,6 @@ let
example =
literalExpression ''"''${pkgs.password-store}/bin/pass borg-repo"'';
};
extraConfig = extraConfigOption;
};

retention = {
Expand All @@ -101,8 +98,6 @@ let
keepWeekly = mkRetentionOption "weekly";
keepMonthly = mkRetentionOption "monthly";
keepYearly = mkRetentionOption "yearly";

extraConfig = extraConfigOption;
};

consistency = {
Expand Down Expand Up @@ -131,9 +126,31 @@ let
];
'';
};

extraConfig = extraConfigOption;
};

extraConfig = extraConfigOption;

imports = [
(mkRenamedOptionModule [ "location" "extraConfig" ] [
"extraConfig"
"location"
])

(mkRenamedOptionModule [ "storage" "extraConfig" ] [
"extraConfig"
"storage"
])

(mkRenamedOptionModule [ "retention" "extraConfig" ] [
"extraConfig"
"retention"
])

(mkRenamedOptionModule [ "consistency" "extraConfig" ] [
"extraConfig"
"consistency"
])
];
};
});

Expand All @@ -148,14 +165,14 @@ let
hmExcludeFile = pkgs.writeText "hm-symlinks.txt" hmExcludePatterns;

writeConfig = config:
generators.toYAML { } {
generators.toYAML { } ({
location = removeNullValues {
source_directories = config.location.sourceDirectories;
repositories = config.location.repositories;
} // config.location.extraConfig;
};
storage = removeNullValues {
encryption_passcommand = config.storage.encryptionPasscommand;
} // config.storage.extraConfig;
};
retention = removeNullValues {
keep_within = config.retention.keepWithin;
keep_secondly = config.retention.keepSecondly;
Expand All @@ -165,10 +182,9 @@ let
keep_weekly = config.retention.keepWeekly;
keep_monthly = config.retention.keepMonthly;
keep_yearly = config.retention.keepYearly;
} // config.retention.extraConfig;
consistency = removeNullValues { checks = config.consistency.checks; }
// config.consistency.extraConfig;
};
};
consistency = removeNullValues { checks = config.consistency.checks; };
} // config.extraConfig);
Copy link
Member

Choose a reason for hiding this comment

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

It may be a bit unfortunate that this doesn't do a recursive update so consistency.extraConfig = … would give a different result compared to extraConfig.consistency = ….

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is a very good point. Would you mind giving me a clue on how to implement that please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after discussing on Matrix, we decided to:

  1. get rid of all the extraConfig options and only keep the top-level one
  2. use mkRenamedOptionModule to warn the user about the deprecated API

Unfortunately, I've been unable to make it work. I pushed a WIP commit to show my latest try. Can someone please help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest issue I have is

error: You're trying to declare a value of type `list'
       rather than an attribute-set for the option
       `programs.borgmatic.backups.<name>.imports'!

       This usually happens if `programs.borgmatic.backups.<name>.imports' has option
       definitions inside that are not matched. Please check how to properly define
       this option by e.g. referring to `man 5 configuration.nix'!
(use '--show-trace' to show detailed location information)

Copy link
Member

Choose a reason for hiding this comment

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

The immediate problem is that you're defining imports inside options, but it should be at the same level as config and options.

The real problem is that these mkRemovedOptionModules will not work because of NixOS/nixpkgs#96006. The most promising attempt to fix that is currently NixOS/nixpkgs#207187.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand what @ncfavier just said, it seems impossible to deprecate the non-top-level extraConfig options for now. Having both a top-level extraConfig option and having non-top-level ones is messy.

The only solution I see is to have no top-level extraConfig option. As a result, I suggest we introduce a top-level option for each of borgmatic's setting sections and add one extraConfig for each. If I'm not mistaken after reading the example configuration file, this just means adding output and hooks top-level options.

What do you think @ncfavier and @rycee?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this approach in #4108.

in {
meta.maintainers = [ maintainers.DamienCassou ];

Expand Down
33 changes: 18 additions & 15 deletions tests/modules/programs/borgmatic/basic-configuration.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,13 @@ in {
location = {
sourceDirectories = [ "/my-stuff-to-backup" ];
repositories = [ "/mnt/disk1" "/mnt/disk2" ];
extraConfig = {
one_file_system = true;
exclude_patterns = [ "*.swp" ];
};
};

storage = {
encryptionPasscommand = "fetch-the-password.sh";
extraConfig = { checkpoint_interval = 200; };
};
storage = { encryptionPasscommand = "fetch-the-password.sh"; };

retention = {
keepWithin = "14d";
keepSecondly = 12;
extraConfig = { prefix = "hostname"; };
};

consistency = {
Expand All @@ -40,8 +32,17 @@ in {
frequency = "4 weeks";
}
];
};

extraConfig = { prefix = "hostname"; };
extraConfig = {
location = {
one_file_system = true;
exclude_patterns = [ "*.swp" ];
};
storage = { checkpoint_interval = 200; };
retention = { prefix = "hostname"; };
consistency = { prefix = "hostname"; };
hooks = { before_actions = [ "echo Starting actions." ]; };
};
};
};
Expand All @@ -65,22 +66,22 @@ in {
builtins.elemAt backups.main.location.repositories 1
}"
expectations[location.one_file_system]="${
boolToString backups.main.location.extraConfig.one_file_system
boolToString backups.main.extraConfig.location.one_file_system
}"
expectations[location.exclude_patterns[0]]="${
builtins.elemAt backups.main.location.extraConfig.exclude_patterns 0
builtins.elemAt backups.main.extraConfig.location.exclude_patterns 0
}"

expectations[storage.encryption_passcommand]="${backups.main.storage.encryptionPasscommand}"
expectations[storage.checkpoint_interval]="${
toString backups.main.storage.extraConfig.checkpoint_interval
toString backups.main.extraConfig.storage.checkpoint_interval
}"

expectations[retention.keep_within]="${backups.main.retention.keepWithin}"
expectations[retention.keep_secondly]="${
toString backups.main.retention.keepSecondly
}"
expectations[retention.prefix]="${backups.main.retention.extraConfig.prefix}"
expectations[retention.prefix]="${backups.main.extraConfig.retention.prefix}"

expectations[consistency.checks[0].name]="${
(builtins.elemAt backups.main.consistency.checks 0).name
Expand All @@ -94,7 +95,9 @@ in {
expectations[consistency.checks[1].frequency]="${
(builtins.elemAt backups.main.consistency.checks 1).frequency
}"
expectations[consistency.prefix]="${backups.main.consistency.extraConfig.prefix}"
expectations[consistency.prefix]="${backups.main.extraConfig.consistency.prefix}"

expectations[hooks.before_actions[0]]="echo Starting actions."

yq=${pkgs.yq-go}/bin/yq

Expand Down