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

discourse: 2.7.9 -> 2.8.0.beta9 #147506

Merged
merged 5 commits into from
Dec 8, 2021
Merged

Conversation

talyz
Copy link
Contributor

@talyz talyz commented Nov 26, 2021

Motivation for this change
  • Update Discourse to the latest beta release, as discussed in Should we package the Discourse Beta branch? #146308
  • Add a libv8 output to nodejs as an alternative to v8, which is a lot of work to update
  • Update Discourse plugins to the latest releases and make it possible to update discourse-prometheus
  • Replace up-plugins.sh with a README pointing to the plugin packaging docs
  • Improve version handling in the update.py script
  • Implement Discourse's plugin pinning algorithm in the update.py script
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/)
  • 21.11 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@talyz talyz requested review from ryantm and dpausp November 26, 2021 15:26
@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 Nov 26, 2021
@talyz talyz force-pushed the discourse-2.8.0.beta8 branch from 2c9da33 to 66725bf Compare November 26, 2021 15:31
@talyz talyz force-pushed the discourse-2.8.0.beta8 branch 3 times, most recently from 1144fcf to 2ec92f7 Compare November 26, 2021 18:37
@bobby285271 bobby285271 linked an issue Nov 29, 2021 that may be closed by this pull request
2 tasks
@talyz talyz mentioned this pull request Nov 29, 2021
13 tasks
@talyz talyz force-pushed the discourse-2.8.0.beta8 branch from 2ec92f7 to 7e08543 Compare November 30, 2021 14:23
Copy link
Member

@ryantm ryantm left a comment

Choose a reason for hiding this comment

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

Code changes look good, as far as I can assess. I don't know much about the v8 changes.

I've upgraded a few of my deployments with it without trouble. Tested the following plugins work: LDAP, data-explorer, solved, checklist, calendar.

@talyz talyz force-pushed the discourse-2.8.0.beta8 branch from 7e08543 to d344e6c Compare November 30, 2021 17:36
@@ -0,0 +1 @@
0.5.0
Copy link
Member

Choose a reason for hiding this comment

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

It is not very obvious why we need this file. Could it be also generated on the fly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's imported from the plugin's repo by the update.py script and read by the Gemfile.

Since building nodejs also builds v8, one way to get a static v8
library is to manually assemble it from the leftover object
files. This seems like an easier way to get an up-to-date v8 library
than trying to keep the v8 package updated.
@talyz talyz force-pushed the discourse-2.8.0.beta8 branch from d344e6c to ea46821 Compare November 30, 2021 22:08
talyz added 3 commits December 2, 2021 10:31
Update to the latest beta, since upstream advocates for it. See
NixOS#146308 for more info.
Also, add support for updating plugins which keep gem versions in
files at the root of the repo (discourse-prometheus) and replace the
`up-plugin.sh` script with a README file pointing to the plugin
packaging documentation.
Add a DiscourseVersion class which handles Discourse's version
numbering properly when sorting - beta versions are sorted lower than
their respective release versions. It can also return both its version
number and equivalent git tag, removing the need for `rev2version` and
manually adding `v` to the front.

Using DiscourseVersion instead of LooseVersion, we can list all
current version number tags from the `discourse` repo and sort them
correctly, giving us the latest one, regardless of type; i.e. we don't
have to filter for only release versions or beta versions anymore.

This also implements the plugin pinning algorithm laid out here:
https://meta.discourse.org/t/pinning-plugin-and-theme-versions-for-older-discourse-installs/156971
to make sure we don't upgrade plugins further than what's compatible
with our currently packaged Discourse version. While it likely won't
matter much most of the time if we continue packaging the beta
versions, it could be helpful if we decide to go back to packaging
release versions or if we run into issues with future upgrades. In
that case, the plugins could still be updated safely even though we're
not on the latest version of Discourse.
@talyz talyz force-pushed the discourse-2.8.0.beta8 branch from ea46821 to 4fb343c Compare December 2, 2021 11:20
@talyz
Copy link
Contributor Author

talyz commented Dec 2, 2021

I've updated to 2.8.0.beta9 and improved the version handling in the update.py script. It should now look for the latest release among all available version numbered releases.

It also now implements Discourse's plugin pinning algorithm to make sure we only update plugins to versions which are compatible with our packaged version of Discourse. While this likely won't matter much most of the time if we continue packaging the beta versions, it could be helpful if we decide to go back to packaging release versions or if we run into issues with future upgrades. In that case, the plugins could still be updated safely even though we're not on the latest version of Discourse.

@talyz talyz changed the title discourse: 2.7.9 -> 2.8.0.beta8 discourse: 2.7.9 -> 2.8.0.beta9 Dec 2, 2021
@ryantm
Copy link
Member

ryantm commented Dec 2, 2021

Cool, @talyz. I deployed your latest thing to my deployments and it seems to be working fine.

Instead of patching the path to /public in Discourse's sources, make
the nginx configuration refer to the symlink in the discourse
package which points to the real path.

When there is a mismatch between the path nginx serves and the path
Discourse thinks it serves, we can run into issues like files not
being served - at least when sendfile requests from the ruby app are
processed by nginx. The issue I ran into most recently is that backup
downloads don't work.

Since Discourse refers to the public directory relative to the Rails
root in many places, it's much easier to just sync this path to the
nginx configuration than trying to patch all occurrences in the
sources. This should hopefully mean less potential for breakage in
future Discourse releases, too.
@talyz
Copy link
Contributor Author

talyz commented Dec 6, 2021

I've added a fix for the /public path issue from #142528 that I think is better than the current patch.

I recently noticed that backup downloads don't work and it turned out that the root cause was the same issue (a mismatch between how nginx and Discourse refer to the same path in sendfile requests). There may be more places we haven't encountered yet, and even more could crop up in the future, so I've fixed the issue by updating the nginx configuration instead.

@talyz talyz linked an issue Dec 6, 2021 that may be closed by this pull request
@ryantm
Copy link
Member

ryantm commented Dec 6, 2021

@talyz that change looks good to me

@talyz
Copy link
Contributor Author

talyz commented Dec 6, 2021

I'll merge this on Wednesday (2021-12-08) unless anyone objects. It would be nice to get some more eyes on the v8 change, but it shouldn't affect nodejs (unless it fails the build), since it doesn't touch any of the default outputs.

Copy link
Contributor

@dpausp dpausp left a comment

Choose a reason for hiding this comment

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

Thanks! Code changes look good to me, upgraded my test system from 2.7.9. Tested basic posting functionality, backups and the following plugins: discourse-assign, discourse-canned-replies, discourse-docs, discourse-spoiler-alert, discourse-voting.

@talyz talyz merged commit 9bf94de into NixOS:master Dec 8, 2021
@talyz talyz deleted the discourse-2.8.0.beta8 branch December 8, 2021 17:15
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2021

Successfully created backport PR #149677 for release-21.11.

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/` 10.rebuild-darwin: 101-500 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulnerability roundup 108: discourse-2.7.9: 2 advisories [7.5] Should we package the Discourse Beta branch?
4 participants