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

nixos/tzupdate: make enabled module actually be enabled #361373

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yajo
Copy link
Contributor

@yajo yajo commented Dec 3, 2024

Without this fix, when setting services.tzupdate.enable = true, the service would never run automatically.

Now, it's actually enabled in systemd and it actually gets executed.

Still, it could be improved with a timer as explained in #127984 (comment), but this makes it at least work out of the box when rebooting the system.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 3, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Eval summary

  • Added packages: 0
  • Removed packages: 0
  • Changed packages: 2
  • Rebuild Linux: 2
  • Rebuild Darwin: 0

@xanderio xanderio added the backport release-24.11 Backport PR automatically label Dec 3, 2024
@xanderio xanderio self-assigned this Dec 3, 2024
@yajo yajo force-pushed the tzupdate-work-out-of-the-box branch from a5c288b to 55898d8 Compare December 11, 2024 09:06
Copy link

@citriqa citriqa left a comment

Choose a reason for hiding this comment

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

The first commit's commit message is outdated with the addition of the second one, maybe that can be updated.

nixos/modules/services/misc/tzupdate.nix Show resolved Hide resolved
@citriqa
Copy link

citriqa commented Jan 18, 2025

Generally, when making changes such as this you will want to rebase and alter existing commits instead of adding another commit, as this keeps the commit history of the project clean once merged and makes it much easier for everyone else to work with after the fact. Rebasing will also allow you to change commit messages in addition to the commit contents. Specifically, you will want to look up how to do interactive rebasing using Git.
In the context of a pull request to a collaborative open source project, the commit history is part of your contribution and functions as documentation, meaning it should receive the same level of care as the code itself instead of reflecting the history of how you arrived at the final set of changes contained within the PR.

@yajo
Copy link
Contributor Author

yajo commented Jan 20, 2025

I maintain other open source projects. Over time I've noticed it takes more time to explain rebases than to click the merge and squash button that github offers, mostly if there's just one meaningful change to record in history. It also makes contribution experience more pleasant because there are less details to care about.

I respect the decision here nevertheless and will rebase.

yajo added 2 commits January 20, 2025 08:48
Without this fix, when setting `services.tzupdate.enable = true`, the service would never run automatically.

Now, it's actually enabled in systemd and it actually gets executed.

Still, it could be improved with a timer as explained in NixOS#127984 (comment), but this makes it at least work out of the box when rebooting the system.
@yajo yajo force-pushed the tzupdate-work-out-of-the-box branch from 98a792d to b2543bf Compare January 20, 2025 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants