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

mattermost: use formats.json; squash systemd-tmpfiles warning #376838

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

Conversation

numinit
Copy link
Contributor

@numinit numinit commented Jan 26, 2025

There was a systemd-tmpfiles warning about not being able to remove the 'plugins' directory. Squash this warning through removal of unnecessary systemd-tmpfiles options, and write a test for it.

Add the {option}services.mattermost.pluginsBundle option to allow overriding the plugin directory and also using it for tests. Update wording in option documentation so it is more clear.

Use formats.json instead of builtins.toJSON so module merging works.

Make the tests go faster by pipelining shutdowns of nodes.

Followup to #208181 which introduced these minor nits.

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/` labels Jan 26, 2025
@numinit numinit force-pushed the mattermost/misc-cleanups branch 2 times, most recently from 32e542e to 8c8385a Compare January 26, 2025 00:40
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 26, 2025
@numinit numinit force-pushed the mattermost/misc-cleanups branch 2 times, most recently from d5eca8b to 4bd073f Compare January 26, 2025 00:52
@@ -756,7 +767,7 @@ in
"d= ${tempDir} 0750 ${cfg.user} ${cfg.group} - -"

# Ensure that pluginDir is a directory, as it could be a symlink on prior versions.
"r- ${pluginDir} - - - - -"
Copy link
Contributor Author

@numinit numinit Jan 26, 2025

Choose a reason for hiding this comment

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

These r- caused the warning ("unable to remove..."). d= will make sure they're a directory instead of a symlink to one.

with pkgs;
map (
plugin:
stdenv.mkDerivation {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No CC needed in these, and we can symlink prior to repacking.

mostlyMutableToplevel: str, mostlyMutable,
def switch_to_specialisation(node, toplevel: str, specialisation: str):
print(f"switch_to_specialisation({node.name!r}, {toplevel!r}, {specialisation!r})", file=sys.stderr)
node.succeed(f"{toplevel}/specialisation/{specialisation}/bin/switch-to-configuration switch || true")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a variety of reasons, the config switch can fail for unrelated reasons to Mattermost if we're on unstable, but Mattermost may still restart. We should just wait for it to come up.

@numinit numinit force-pushed the mattermost/misc-cleanups branch from 4bd073f to c896ae1 Compare February 1, 2025 21:53
@numinit numinit changed the title nixos/mattermost: squash systemd-tmpfiles warning nixos/mattermost: use formats.json; squash systemd-tmpfiles warning Feb 1, 2025
@numinit numinit force-pushed the mattermost/misc-cleanups branch 2 times, most recently from d4d14d2 to 59251bd Compare February 1, 2025 22:04
@numinit
Copy link
Contributor Author

numinit commented Feb 1, 2025

Rebased against master and added formats support.

@numinit numinit added the 8.has: tests This PR has tests label Feb 1, 2025
@numinit numinit requested a review from drupol February 20, 2025 04:39
@numinit numinit force-pushed the mattermost/misc-cleanups branch from 59251bd to 79bdf00 Compare February 20, 2025 08:24
@numinit numinit requested a review from drupol February 24, 2025 03:13
@numinit
Copy link
Contributor Author

numinit commented Mar 1, 2025

Should be good to go.

Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

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

Diff LGTM, the tests should build fine for me but we'll see.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 11, 2025
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Mar 12, 2025
@RossComputerGuy
Copy link
Member

image
Tests are still running

@numinit numinit changed the title nixos/mattermost: use formats.json; squash systemd-tmpfiles warning mattermost: use formats.json; squash systemd-tmpfiles warning Mar 13, 2025
@numinit numinit force-pushed the mattermost/misc-cleanups branch from 79bdf00 to 0ed8a55 Compare March 13, 2025 05:13
@numinit
Copy link
Contributor Author

numinit commented Mar 13, 2025

Rebased, let's try that.

@RossComputerGuy
Copy link
Member

Alright, trying it now.

There was a systemd-tmpfiles warning about not being able to remove the
'plugins' directory. Squash this warning through removal of unnecessary
systemd-tmpfiles options, and write a test for it.

Add the {option}`services.mattermost.pluginsBundle` option to allow
overriding the plugin directory and also using it for tests. Update
wording in option documentation so it is more clear.

Use formats.json instead of builtins.toJSON so module merging works.

Make the tests go faster by pipelining shutdowns of nodes.
@numinit numinit force-pushed the mattermost/misc-cleanups branch from 0ed8a55 to e4d40ca Compare March 13, 2025 06:03
@numinit
Copy link
Contributor Author

numinit commented Mar 13, 2025

Changed nixos/mattermost -> mattermost in the commit message so eval works.

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Mar 13, 2025
@numinit
Copy link
Contributor Author

numinit commented Mar 14, 2025

Seems to fail on aarch64 with:

--- FAIL: TestUploadFiles/simple_Happy_image_thumbnail/preview_6_[orientation_test_6.jpeg] (1.76s)
FAIL channels/api4.TestUploadFiles/simple_Happy_image_thumbnail/preview_6_[orientation_test_6.jpeg] (re-run 1) (1.76s)

So the thumbnail generation for posts is nondeterministic on aarch64...

@numinit
Copy link
Contributor Author

numinit commented Mar 14, 2025

These were added fairly recently, and apparently test that thumbnailing
an image exactly matches a reference image, which varies by arch.
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/` 8.has: tests This PR has tests 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants