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/typesense: init at 0.24.1 #244233

Merged
merged 1 commit into from
Jul 22, 2023
Merged

Conversation

oddlama
Copy link
Contributor

@oddlama oddlama commented Jul 18, 2023

Description of changes
  • This adds a package for typesense (a fast and typo-resistant search engine)
  • Adds an accompanying nixos module (RFC42 compatible).
  • Adds nixos tests for the module
  • Closes Please add typesense  #202136

I tried my best to not use a binary distribution here, but after trying to make it work (for far too long) I have decided that it isn't currently feasible. (Typesense will switch to bazel in the next version and creating a working build is almost impossible. You cannot prevent it from downloading stuff at build time; It fails if LANG is not C.UTF-8; The bazel sandbox prevents access to tools built with nix like cmake; ...) If you know how to make it work, please ping me on matrix.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 18, 2023
@oddlama oddlama added 8.has: package (new) This PR adds a new package 8.has: module (new) This PR adds a module in `nixos/` and removed 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 18, 2023
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 labels Jul 18, 2023
@github-actions github-actions bot added the 8.has: module (update) This PR changes an existing module in `nixos/` label Jul 19, 2023
@oddlama oddlama force-pushed the init-typesense-bin branch 2 times, most recently from 72a66f5 to aa44460 Compare July 19, 2023 19:41
@oddlama
Copy link
Contributor Author

oddlama commented Jul 19, 2023

added updater script and passthrou for tests. This is now ready for review from my side.

@oddlama oddlama mentioned this pull request Jul 22, 2023
16 tasks
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Minor changes, the work seems HQ as often :).

pkgs/servers/search/typesense/default.nix Outdated Show resolved Hide resolved
pkgs/servers/search/typesense/default.nix Show resolved Hide resolved
pkgs/servers/search/typesense/default.nix Show resolved Hide resolved
@oddlama
Copy link
Contributor Author

oddlama commented Jul 22, 2023

Added comments about issues with building from source, and added requested formatting fixes.

Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Awesome

@RaitoBezarius
Copy link
Member

@ofborg test typesense

@RaitoBezarius RaitoBezarius merged commit c4ae174 into NixOS:master Jul 22, 2023
8 of 9 checks passed
@happysalada
Copy link
Contributor

happysalada commented Sep 11, 2023

hey @oddlama thanks a lot for this!
a couple of suggestions:

Also one question, why did you set the Group and the permission for the state directory to 0700 ? I understand why you would set user, in order to be able to sudo su and restore a backup, but I don't understand what group provides additional to that ?

last but not least, those are all detail and none of them are urgent/mandatory, so feel free to skip if you don't have the time.
(I'd be happy to be pinged for future updates on typesense if you are looking for more reviewers).

@oddlama
Copy link
Contributor Author

oddlama commented Sep 23, 2023

* upstream has a systemd service file (maybe it was added recently). that adds 2 parameters not present here that might be useful https://github.com/typesense/typesense/blob/main/debian-pkg/typesense-server/etc/systemd/system/typesense-server.service (LimitNOFILE=64000
  LimitMEMLOCK=infinity) . They use Require as well on top of After (I know using require is debatable, so feel free to skip if you disagree, I don't think this option is worth fighting for).

It looks me like they copied this from some template service without thinking about whether it is necessary. Their whole codebase never uses mlock so loosening the limit to infinity has no effect while making the sandbox less strict. Also the default LimitNOFILE is 1024 soft and ~500k hard, so I'm not sure why 64k should be beneficial. I'm open to changing this if there's a rationale.

Also one question, why did you set the Group and the permission for the state directory to 0700 ? I understand why you would set user, in order to be able to sudo su and restore a backup, but I don't understand what group provides additional to that ?

I'm not sure I understand the question. The Group is specified so that the group name is explicitly specified instead of being derived by the service name. Due to DynamicUser there will always be a User and Group allocated for the service, we just set the name for both.

Regarding mode 0700, if your point is that the state directory should be 0750 to allow members of the group to do backups, I agree that this should be changed. I initially chose 0700 as a strict default without considering backup services.

I'm unfortunately currently a little short on time, so feel free to make the changes if you need them. Otherwise I'll try to cycle back to this later.

@happysalada
Copy link
Contributor

none of these changes are urgent, so feel free to address when you have time.

The question around the Group, is why do you want to specify the Group, aren't we just fine, leaving whatever systemd chooses ? I understand the rationale for setting the name for the User, I don't see why we explicitely need to set the name for the group.

Regarding mode 0700, my initial thought was that we don't need to set the permissions at all on the directory.
If we define the user name, you can always sudo su as that user and then make the backups as that user.
if indeed you want to use the group to let another user make the backup, then defining a group name and changing the permissions does indeed make sense.

I personally always use the same user to make the backups. I've never seen any argument for or against having a group. I was thinking that you might have good reasons for using the group and was curious about those.

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: changelog 8.has: documentation 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 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.

Please add typesense
3 participants