-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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/suricata: Fix module and add to module-list #349826
base: master
Are you sure you want to change the base?
Conversation
7a37d0c
to
27e132a
Compare
You might want to remove |
9521404
to
83c2bc9
Compare
7b7a965
to
394ce02
Compare
Commit "nixos/suricata: add module to modules-list" is now doing a lot more. I suggest to break this PR down into these commits:
|
394ce02
to
ff1f9e5
Compare
Please mind the order of commits. Each commit should be atomic and take the code from one working state to the next. Currently, the first commit would break the test. |
You may reference the test in https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/networking/ids/suricata/default.nix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the description could be a bit more elaborate and more a full sentence but other than that looking good
ff1f9e5
to
5974cf8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to remove import = [ [...] ../modules/services/networking/suricata/default.nix ] from nixos/tests/suricata.nix (in a separate commit).
WHy in a separate commit? This gives you one commit where the test doesn't evaluate.
the description could be a bit more elaborate and more a full sentence but other than that looking good
I don't think single-word descriptions are appropriate, agreed.
Also, I noticed that the module has some import-from-derivation which made me build erlang while evaluating nixos/release.nix
. Please remove that as well.
Also I must wonder why this got merged in this state in the first place 🤔
I suggested a commit order here: #349826 (comment) (no breakage). |
668e212
to
7d0dd56
Compare
Not quiet sure what to put in there yet. Some of them have good descriptions in https://docs.suricata.io/en/suricata-7.0.7/configuration/suricata-yaml.html, which I am going to adopt, but some of them aren't really documented that well. |
7d0dd56
to
740790d
Compare
1b5c54f
to
15e3ae5
Compare
15e3ae5
to
e420853
Compare
Commit |
I forgot to add the suricata module (#313671) to the modules-list, so it doesn't appear in the search.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.