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

[23.11] nixos/snapper: Add peristentTimer option to config #312549

Merged
merged 1 commit into from
May 21, 2024

Conversation

mrkline
Copy link
Contributor

@mrkline mrkline commented May 17, 2024

Defaults to false, but allows users to enable it for machines that aren't on persistently (e.g., laptops, home PCs).

Backport of 2c55e03 (#306909) but using lib.mdDoc to be consistent with the rest of the options on this branch.

@mrkline
Copy link
Contributor Author

mrkline commented May 17, 2024

@tomberek if you've got the time to take a look over the backport as well, much appreciated!

@jian-lin
Copy link
Contributor

Thanks for your contribution.

Here is some doc for manual backport: https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#manually-backporting-changes. Could you make some changes such as PR title and git cherry-pick -x according to that?

using lib.mdDoc to be consistent with the rest of the options on this branch

This is indeed an disadvantage of auto backport, which I think is acceptable. If you want an auto backport, I can help with that.

@mrkline
Copy link
Contributor Author

mrkline commented May 20, 2024

AIUI cherry-pick -x adds a line referencing the commit it's coming from - I've done that (and referenced the PR).

And it's not a cherry-pick (neither manual nor auto-backported), as the commit message notes - I've changed it to match the rest of the module's args in 23.11

@jian-lin jian-lin changed the title nixos/snapper: Add peristentTimer option to config [23.11] nixos/snapper: Add peristentTimer option to config May 20, 2024
@jian-lin
Copy link
Contributor

AIUI cherry-pick -x adds a line referencing the commit it's coming from - I've done that (and referenced the PR).

I see you link that PR in the description.

What I ask is that you also reference the backported commit hash ff0f4540c088913db38d9a5006b4a85e768c9ae5 in the commit message like what git cherry-pick -x gives you.

as the commit message notes - I've changed it to match the rest of the module's args in 23.11

I see you add lib.mkDoc here.

I suggest an auto-backport because the addition of lib.mkDoc is, in my opinion, only cosmetic and the auto-backport references to both the PR and the commit hash properly.

And it's not a cherry-pick (neither manual nor auto-backported)

Usually, a manual backport indicates modifying the cherry-picked commit, either in place or in a new commit.

Could you make some changes such as PR title

Not sure if you missed this suggestion or you intend to not do this. Anyway, I have changed it because I see no harm of it. Hope you do not mind.

Defaults to false, but allows users to enable it for machines that
aren't on persistently (e.g., laptops, home PCs).

Backport of NixOS#306909

(cherry picked from commit ff0f454)
@mrkline
Copy link
Contributor Author

mrkline commented May 21, 2024

What I ask is that you also reference the backported commit hash

The PR description contains that as well; I assumed that it would be used as the commit message when the PR is merged.

I suggest an auto-backport

I'm not a maintainer (yet!), so I don't think I have the powers to do so.

I force-pushed a commit generated from cherry-pick -x, referencing the previous commit and PR. I hope that addresses your feedback - apologies for any misunderstanding!

type = types.bool;
example = true;
description = ''
Set the `persistentTimer` option for the
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should it be Persistent? If so, this (and the one in the borgbackup module) can be fixed in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with #314099

@jian-lin
Copy link
Contributor

The PR description contains that as well; I assumed that it would be used as the commit message when the PR is merged.

It is not the case for two of three merge methods on GitHub.

I'm not a maintainer (yet!), so I don't think I have the powers to do so.

I know and that's why I say I can help with that (adding the needed tag). I should have stated that more clear.

Again, thanks.

@jian-lin jian-lin merged commit 8fcbc51 into NixOS:release-23.11 May 21, 2024
18 checks passed
pennae pushed a commit that referenced this pull request May 23, 2024
The persistentTimer argument sets the _Persistent_ field in
systemd.timer(5).

Pointed out in #312549
github-actions bot pushed a commit that referenced this pull request May 23, 2024
The persistentTimer argument sets the _Persistent_ field in
systemd.timer(5).

Pointed out in #312549

(cherry picked from commit 234f4db)
Sobte pushed a commit to Sobte/nixpkgs that referenced this pull request May 28, 2024
The persistentTimer argument sets the _Persistent_ field in
systemd.timer(5).

Pointed out in NixOS#312549
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants