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

outline: 0.68.1 -> 0.69.2 #232235

Merged
merged 4 commits into from May 23, 2023
Merged

Conversation

xanderio
Copy link
Contributor

Description of changes

Changelog: https://github.com/outline/outline/releases

Based on #228101 and update to latest version.

As database migrations are now run automatically on application startup, the corresponding functionally was removed from the preStart unit config.

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.05 Release Notes (or backporting 22.11 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.

@xanderio
Copy link
Contributor Author

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

1 package blacklisted:
  • nixos-install-tools
1 package built:
  • outline

@RaitoBezarius
Copy link
Member

We are unfortunately too close the release for this PR, also, it should add release notes and instructions for the automatic migrations that can cause problems to the database.
And alas, we do not have Outline NixOS tests as far as I see it. Please wait for the branch-off (22th May).

@RaitoBezarius RaitoBezarius marked this pull request as draft May 16, 2023 14:21
@cab404
Copy link
Member

cab404 commented May 16, 2023

We are unfortunately too close the release for this PR, also, it should add release notes and instructions for the automatic migrations that can cause problems to the database.
And alas, we do not have Outline NixOS tests as far as I see it. Please wait for the branch-off (22th May).

Apparently, automatic migrations don't need any instructions (they are automatic), and a warning on 0.69.0 release points towards rolling latest users who use non-extensively tested versions

@RaitoBezarius
Copy link
Member

Apparently, automatic migrations don't need any instructions (they are automatic)

Yes, but the thing about stabilization is that automatic migrations are usually not automatic in their failure modes. :) (we had the situation with multiple other Node.js software such as Hedgedocs — deleting all data at migration time).

and a warning on 0.69.0 release points towards rolling latest users who use non-extensively tested versions

Which our users will not really see because outline does not really make any use of stateVersion.

As said, this PR is too close to the release window, breaking changes were restricted since a while (according to the release schedule calendar), and maybe Outline does not use 0.x versioning like this, but this is a major version bump technically.

Feel free to convince me it has to be merged before we will branch-off (in 5 days) or convince to backport it after release.

@xanderio
Copy link
Contributor Author

I've added a very basic but much needed nixosTest for outline. The first version of this PR was broken at runtime, which I only noticed in production (much fun was had).

The issue was that outline was trying to open share/outline/build/build/app/manifest.json. To fix this I've reworked the install phase so that we are much closer to what the upstream Dockerfile produces. This was already suggested in #217025 (comment)

After fixing this another problem appeared Sequelize was complaining that the sql dialect wasn't specified. This resulted from the generated database.json config file containing a production environment config, but not the production-ssl-disabled environment.

I've manually verified that #217025 doesn't appear again.

@xanderio
Copy link
Contributor Author

That's a bit unfortunate the nixos test in passthru.tests is executed by ofborg, as outline is under a unfree license. Is there any way around this?

@cab404
Copy link
Member

cab404 commented May 18, 2023

That's a bit unfortunate the nixos test in passthru.tests is executed by ofborg, as outline is under a unfree license. Is there any way around this?

Actually building and redistributing it surely falls under a fair use by the license terms, but we don't really have distinction between «unfree as in nvidia drivers» and «unfree as don't sell it»

Should probably make a nice talking point for nixpkgs committee

@RaitoBezarius
Copy link
Member

That's a bit unfortunate the nixos test in passthru.tests is executed by ofborg, as outline is under a unfree license. Is there any way around this?

Actually building and redistributing it surely falls under a fair use by the license terms, but we don't really have distinction between «unfree as in nvidia drivers» and «unfree as don't sell it»

Should probably make a nice talking point for nixpkgs committee

While it would be nice, it would create a certain legal risk and we do not really want to have a DMCA on the nixpkgs repository someday. Except if you get some explicit exception, I don't think it's really feasible.

We have a redistributable flag IIRC.

@cab404
Copy link
Member

cab404 commented May 18, 2023

We have a redistributable flag IIRC.

(asking @tommoor for explicit permission)

Its license is quite short and concise — and does give explicit redistribution permission.

Let's maybe add redistributable then?

@tommoor
Copy link

tommoor commented May 19, 2023

This seems likely fine – how is the license presented to users of NixOS?

@cab404
Copy link
Member

cab404 commented May 19, 2023

This seems likely fine – how is the license presented to users of NixOS?

They are getting warned if they are using anything with an unfree license
image

@RaitoBezarius
Copy link
Member

I'm sorry, no expert in licenses, but we strive to make it right:
I read:

The Licensor hereby grants you the right to copy, modify, create derivative
works, redistribute, and make non-production use of the Licensed Work.

"make non-production use of the Licensed Work", but that's exactly what is about the NixOS module for Outline, aren't we?

We can certainly redistribute the binaries, but I don't know about who is responsible for the "make non-production use of", NixOS modules are production-grade deployment recipes for anything…

@cab404
Copy link
Member

cab404 commented May 21, 2023

We can certainly redistribute the binaries, but I don't know about who is responsible for the "make non-production use of", NixOS modules are production-grade deployment recipes for anything…

Well, all of the work performed on Hydra is not in violation of the license, and users are having to agree to the terms. We really cannot ensure their compliance to them though.

@tommoor
Copy link

tommoor commented May 21, 2023

Well, all of the work performed on Hydra is not in violation of the license, and users are having to agree to the terms. We really cannot ensure their compliance to them though.

Agreed. Neither can we when downloaded straight from GitHub, just a matter of enforcement as we find the occasional infringement. The non-free warning should suffice.

@xanderio
Copy link
Contributor Author

@cab404 @yrd May I add myself to the maintainers of the nix package?

@cab404
Copy link
Member

cab404 commented May 23, 2023

@xanderio can you rebase this, so nixos.tests would run?)
redistributable tag got merged)

@cab404
Copy link
Member

cab404 commented May 23, 2023

Hm, apparently we can't build test machines with redistributable flag only :/

time for another PR)

@cab404
Copy link
Member

cab404 commented May 23, 2023

image
There are several tests using allowUnfree in the config, alas I don't personally think that's a great idea.

But for the sake of saving time, we can do that for now

@xanderio
Copy link
Contributor Author

For some reason we're using nodejs 16, which has been marked insecure by #229910. I've pushed another commit that should fix this issue.

@RaitoBezarius RaitoBezarius marked this pull request as ready for review May 23, 2023 12:36
@RaitoBezarius
Copy link
Member

Once CI passes, let's get it.

@xanderio
Copy link
Contributor Author

CI has passed 🎉

@RaitoBezarius RaitoBezarius merged commit 078d3eb into NixOS:master May 23, 2023
22 checks passed
@xanderio xanderio mentioned this pull request May 23, 2023
12 tasks
@yrd
Copy link
Member

yrd commented May 25, 2023

Nice, thank you!

@xanderio xanderio deleted the outline-0.69.2 branch May 25, 2023 10:25
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