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

Octoprint: hardening and RFC 42 #335827

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

PatrickDaG
Copy link
Contributor

@PatrickDaG PatrickDaG commented Aug 19, 2024

Description of changes

I've added rfc 42 compliant options to the octoprint module, which in my opinion make it easier to read and understand.
Additionally I've added some basic systemd hardening for the service and remove the default enabled raspberryPi specific option. This might break some setup but previously for none raspberry Pi setups there was no option to remove the tempfiles.
Lastly I removed the ffmpeg patch from the package. This breaks timelapses for people using the package without the module, who I expecte to be able to, or even want to set their own ffmpeg, something that wasn't possible previously.

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.11 Release Notes (or backporting 23.11 and 24.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/` labels Aug 19, 2024
@PatrickDaG PatrickDaG changed the title Octoprint: hardening and RFC Octoprint: hardening and RFC 42 Aug 19, 2024
Copy link
Member

@gador gador left a comment

Choose a reason for hiding this comment

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

Since octoprint was developed with raspberypi in mind, I would suggest adding a release note about the enableRaspberryPi option.
Other than that: This looks good to me 👍

@gador
Copy link
Member

gador commented Aug 20, 2024

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

Result of nixpkgs-review pr 335827 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
2 packages built:
  • octoprint
  • octoprint.dist

@gador
Copy link
Member

gador commented Aug 20, 2024

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

Result of nixpkgs-review pr 335827 run on aarch64-linux 1

1 package blacklisted:
  • nixos-install-tools
2 packages built:
  • octoprint
  • octoprint.dist

@PatrickDaG
Copy link
Contributor Author

I've added a release note.

@ofborg ofborg bot requested a review from gador August 21, 2024 10:22
@gador
Copy link
Member

gador commented Aug 21, 2024

I'm ok with this 👍

@abbradar @gebner @WhittlesJr what do you think?

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 8, 2024
Copy link
Member

@Scrumplex Scrumplex left a comment

Choose a reason for hiding this comment

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

Because of the freeze of breaking changes, I'd recommend adding a state version condition for the Raspberry Pi option.

LGTM otherwise

};
enableRaspberryPi = mkEnableOption "RaspberryPi specific hardware access rules";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enableRaspberryPi = mkEnableOption "RaspberryPi specific hardware access rules";
enableRaspberryPi = mkEnableOption "RaspberryPi specific hardware access rules" // {
default = lib.versionOlder config.system.stateVersion "25.05";
};

@PatrickDaG PatrickDaG force-pushed the octoprint-update branch 2 times, most recently from 2195eb2 to fcdd2ee Compare October 28, 2024 16:35
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 1, 2024
ffmpeg is only used for timelapses which will just be disabled if the
options isn't set. Additionally this allows people to ship their own
ffmpeg.
@PatrickDaG PatrickDaG marked this pull request as draft November 3, 2024 21:07
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Nov 4, 2024
@PatrickDaG PatrickDaG added the 2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: wait for branch‐off Waiting for the next Nixpkgs branch‐off 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants