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/installation-device: remove warning about mdadm #273308

Merged
merged 1 commit into from
Dec 25, 2023

Conversation

Stunkymonkey
Copy link
Contributor

Description of changes

discussion from #254807

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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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/` labels Dec 10, 2023
Copy link
Contributor

@ctheune ctheune left a comment

Choose a reason for hiding this comment

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

Jeesh, I hate mdadm.conf format.

Double checking the man page it shows the syntax without =:

 PROGRAM /usr/sbin/handle-mdadm-events

Did you test your change? I can imagine mdadm being lax about the syntax, but still ...
Is there an existing test you could augment?

@Stunkymonkey Stunkymonkey force-pushed the install-device-fix-mdadm branch from f991df4 to 9537527 Compare December 10, 2023 12:06
@Stunkymonkey
Copy link
Contributor Author

@ctheune I adjusted the config as you showed.

I did not test this.

@ctheune
Copy link
Contributor

ctheune commented Dec 10, 2023

Changing the mdadm stuff has been a bit of a mixed blessing recently and test coverage is quite low.

Please test your changes (we have a check list for that) and also help against future regressions by adding it to e.g. nixos/modules/installer/cd-dvd/installation-cd-base.nix or nixos/tests/installer.nix

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 10, 2023
@lheckemann
Copy link
Member

lheckemann commented Dec 11, 2023

Given that this is only for installer images, I think it's fine for it not to be tested beyond the existing installation tests -- Maybe it would even make sense to disable the mdmonitor service completely in addition?

@ofborg test installer.swraid

@ctheune
Copy link
Contributor

ctheune commented Dec 11, 2023

Disabling the monitor service is basically impossible due to the convoluted structure of the upstream udev rules.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 11, 2023
@Stunkymonkey
Copy link
Contributor Author

ping?

@lheckemann lheckemann merged commit 5494aa2 into NixOS:master Dec 25, 2023
26 checks passed
Copy link
Contributor

github-actions bot commented Jan 1, 2024

Successfully created backport PR for release-23.11:

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 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants