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 8.1.10 → 9.5.1 #291409

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Conversation

stuebinm
Copy link
Contributor

@stuebinm stuebinm commented Feb 25, 2024

Description of changes

This jumps Mattermost ESR Versions (see here for their release cycle). The new version makes use of Go's workspace feature, which unfortunately the buildGoModule function does not (yet?) support, which breaks our previous build process for mattermost.

Further, the new release also makes use of private modules only included in the (non-free) enterprise version of mattermost which makes it impossible to build in the usual way even outside of nixpkgs's build abstractions if vendoring is required.

Both issues can be solved by using Go 1.22, which has added support for vendoring when using workspaces, and instructing it to ignore missing module errors with the -e flag (this works via the new go work vendor command; the "missing" modules turn out to be unimportant during the build, so it still succeeds). This requires overriding the go-modules derivation's buildPhase via overrideModAttrs. The resulting binary passes the nixos tests, and I have deployed on a production server without issues.

Additionally, @emilylange showed me how to use passthru.updateScript, so this PR incorporates this as well. Previously, the versions of mattermost in nixpkgs always tended to lag behind upstream's released versions a little by a minor (security-patch) version or two, and it'd be nice to improve this situation & hopefully this will help.

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
      • Note on this: I'm not sure what constitutes "major" here — is it enough that e.g. downgrading might be hard after a database migration?
    • (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.

This jumps Mattermost ESR Versions (see [1] for their release cycle). The
new version makes use of Go's workspace feature, which unfortunately the
buildGoModule function does not (yet?) support [2], which breaks the
previous build process for mattermost.

Further, the new release also makes use of private modules only included
in the (non-free) enterprise version of mattermost which makes it impossible
to build in the usual way even outside of nixpkgs's build abstractions [3].

Both issues can be solved by using Go 1.22, which has added support for
vendoring when using workspaces, and instructing it to ignore errors with
the -e flag. This requires overriding the go-modules derivation's
buildPhase, and still produces a functional binary.

[1] https://docs.mattermost.com/upgrade/extended-support-release.html
[2] NixOS#203039
[3] mattermost/mattermost#26221
Previously, updates of mattermost tended to lag behind upstream's
released versions by a minor (security-patch) version or two.

This should hopefully make the r-ryantm bot detect future updates
automatically. Thanks to Emily Lange for explaining to me how this
works!
@stuebinm
Copy link
Contributor Author

oh, one more question: adding "cmd/mmctl" to the subPackages attribute of this package builds a working version of the mmctl cli admin tool, which is currently packaged separately in nixpkgs although it lives in the same upstream git repository as mattermost itself.

Is there any benefit to having it separate, or would it make sense to merge these two packages & add an alias for the mmctl package onto mattermost?

(cc @mgdelacroix @ppom0 since you are maintainers of the mmctl package)

@ppom0
Copy link
Contributor

ppom0 commented Feb 26, 2024

hi @stuebinm, when I added mmctl to nixpkgs, it was in its own repo, so it's a historical reason. however, I see a disk space benefit in keeping it separated: mmctl is genereally used on the admin's desktop, not on the instance server, so the mattermost binary would be useless there. what do you think?

@numinit
Copy link
Contributor

numinit commented Feb 26, 2024

Does the NixOS test for Mattermost still pass? :-)

In other news I'd like to get this one done for 24.05, since that has an integration test for plugin support:

#208181

@stuebinm
Copy link
Contributor Author

@ppom0 hm, I see, that's indeed a reason. But perhaps it would be an option to define the mmctl package as an override / variant of the mattermost package, so there's no doubled effort in maintaining / updating them? (especially seeing how their versions are also currently out of sync)

@emilylange
Copy link
Member

But perhaps it would be an option to define the mmctl package as an override / variant of the mattermost package, so there's no doubled effort in maintaining / updating them?

grafana-loki and promtail do this, just for reference :)

@ppom0
Copy link
Contributor

ppom0 commented Feb 27, 2024

Yes, it would be great indeed!

@stuebinm
Copy link
Contributor Author

should i just do that here in this PR as well? (feels like a slightly different thing, so if it's preferred I'd be happy to open a second PR after this one's been merged)

also @ppom0: while doing that, should I add you as maintainer for mattermost, since maintaining mmctl and mattermost would be one and the same afterwards?

@emilylange
Copy link
Member

should i just do that here in this PR as well? (feels like a slightly different thing, so if it's preferred I'd be happy to open a second PR after this one's been merged)

I would recommend keeping this PR to the bump itself and then creating another PR for other changes, such as making mmcli depend on the mattermost derivation.

Will hit the merge button now, but let this not prevent you from discussing details about the mmcli-thingy with one another :)

@emilylange emilylange merged commit 972f713 into NixOS:master Feb 27, 2024
31 checks passed
@ppom0
Copy link
Contributor

ppom0 commented Feb 27, 2024

@stuebinm I prefer not to be added as a maintainer, thanks for asking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants