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/hedgedoc: refactor to reduce option count #244941

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

h7x4
Copy link
Member

@h7x4 h7x4 commented Jul 23, 2023

Description of changes

Hello!

I've deleted most of the option declarations in settings, reducing the line count from ~1000 to ~350 ~300 (thanks Sandro!). Some of them were old and didn't have any effect anymore, and there are also some new undeclared ones.

I'm a little bit unsure about setting the database to sqlite as default, but it has the benefit that you can now only write services.hedgedoc.enable = true, and be up and running in no time.

The systemd unit has also been hardened.

To address some backwards compatability concerns:

  • A lot of the *Path options where we had overridden the default to point at ${cfg.package}/public/* have been removed, and they have not had any effect since March 2019 in CodiMD 1.3.0. In order for this to affect you, you would have to use an older version of hedgedoc, and not have set the options. If you have a reason to use such an old version while still using the new module, I'd assume you probably know what you're doing (maybe).
  • I believe there's technically an undisclosed breaking change, where services.hedgedoc.settings.ldap.tlsca has been overriden to default to /etc/ssl/certs/ca-certificates.crt. I do not think this warrants messing around with mkRemovedOptionModule workarounds for submodules.
  • Deprecating workDir should have limited effect. uploadsPath' default was defined in terms of workDir, but it should still point to the same location. I've added a warning about the case where someone were to override workDir without touching uploadsPath. The other directories which would normally exist here (hedgedoc seems to assume its workdir to be its package), have been overriden to point directly into the package, so there should be no worries about these. HedgeDoc doesn't seem to store anything else here, as the rest is stored inside its database.

I also want to raise the question about when, if ever, we could start removing the name duality in the module, but I think I will leave it as it is for this PR.

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.

I've tested the module using nix-build -A nixosTests.hedgedoc and I've also been using the systemd hardening for about a week now. It has been working fine so far.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Nice and much needed refactor ! Looking forward to use it :)

Link to RFC42: https://github.com/NixOS/rfcs/blob/master/rfcs/0042-config-option.md

It would be nice to fix the broken eval though.

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

This has to wait until the next update which is scheduled for the coming days.

nixos/tests/hedgedoc.nix Outdated Show resolved Hide resolved
nixos/tests/hedgedoc.nix Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-apps/hedgedoc.nix Outdated Show resolved Hide resolved
@SuperSandro2000 SuperSandro2000 marked this pull request as draft July 24, 2023 19:55
@h7x4 h7x4 force-pushed the reduce-options-in-hedgedoc-module branch 2 times, most recently from de5fd4a to 5ade396 Compare July 24, 2023 21:45
@h7x4
Copy link
Member Author

h7x4 commented Jul 24, 2023

This has to wait until the next update which is scheduled for the coming days.

This is a bit unclear to me. What exactly is updating in a few days? Hedgedoc?

@SuperSandro2000
Copy link
Member

This is a bit unclear to me. What exactly is updating in a few days? Hedgedoc?

Yes :)

@h7x4 h7x4 force-pushed the reduce-options-in-hedgedoc-module branch 6 times, most recently from 2b96658 to e25c17f Compare July 26, 2023 18:04
@h7x4 h7x4 force-pushed the reduce-options-in-hedgedoc-module branch from e25c17f to 59e624a Compare July 28, 2023 15:14
@h7x4 h7x4 marked this pull request as ready for review July 31, 2023 08:26
@h7x4
Copy link
Member Author

h7x4 commented Jul 31, 2023

Reopening in light of #246259

@drupol
Copy link
Contributor

drupol commented Aug 2, 2023

Looking forward for this PR !

@h7x4 h7x4 force-pushed the reduce-options-in-hedgedoc-module branch from 59e624a to 1b22721 Compare August 2, 2023 18:03
@h7x4
Copy link
Member Author

h7x4 commented Aug 3, 2023

Okay, so here's the deal.

I've played around with the ofborg eval a bit, and I found out that the reason behind the ofborg eval traces is that the manual itself depends on whether we use the name "codimd" or "hedgedoc" (which again depends on stateVersion). This is bad, because the manual is rendered without anyone setting any stateVersion, and it is also just cursed.

I'll remove the name from the relevant places and just say "hedgedoc" where needed, but I really think we should get rid of the name duality sooner rather than later.

@h7x4 h7x4 force-pushed the reduce-options-in-hedgedoc-module branch from 18d4f36 to 720f3d9 Compare August 13, 2023 13:26
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2571

@h7x4 h7x4 requested a review from winterqt August 23, 2023 22:48
@Artturin
Copy link
Member

Is a release note needed?

Copy link
Contributor

@adamcstephens adamcstephens left a comment

Choose a reason for hiding this comment

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

I've applied this to my install and it works as expected.

As a sqlite user, I appreciate the default sqlite support. :) I was able to delete a few lines.

@h7x4
Copy link
Member Author

h7x4 commented Oct 15, 2023

@Artturin I've now added a release note

- Remove lots of declared options that were not used outside of being
  included in settings. These should now be used through the freeform
  module.
- Deprecate `cfg.workDir`, in favor of using systemds `StateDirectory`
- Use sqlite as default database.

Co-authored-by: Sandro Jäckel <sandro.jaeckel@gmail.com>
adamcstephens added a commit to adamcstephens/nixpkgs that referenced this pull request Oct 16, 2023
adamcstephens added a commit to adamcstephens/nixpkgs that referenced this pull request Oct 16, 2023
adamcstephens added a commit to adamcstephens/nixpkgs that referenced this pull request Oct 16, 2023
@h7x4 h7x4 force-pushed the reduce-options-in-hedgedoc-module branch from 53fa77c to 746b319 Compare October 16, 2023 18:31
@h7x4 h7x4 force-pushed the reduce-options-in-hedgedoc-module branch from 746b319 to abe4688 Compare October 16, 2023 18:38
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

didn't test this yet but if others tested it than it is probably fine

@drupol drupol merged commit fdecb1d into NixOS:master Oct 20, 2023
19 checks passed
@h7x4 h7x4 deleted the reduce-options-in-hedgedoc-module branch November 9, 2023 15:11
@bgamari
Copy link
Contributor

bgamari commented Dec 3, 2023

I have found that the hardening commit broke my SQLite-based installation upon upgrading to NixOS-23.11. Specially, it appears that the daemon is no longer able to open the database. Unfortunately I do not have time to investigate at the moment so I have reverted locally. I might suggest that upstream do the same to prevent others being put in this situation.

@SuperSandro2000
Copy link
Member

I might suggest that upstream do the same to prevent others being put in this situation.

We don't need to hastily revert everything, just that portion.

Is you sqlite in the default location? hedgedoc should then be able to read that. Also we can probably get the path from the settings, too.

@h7x4
Copy link
Member Author

h7x4 commented Dec 13, 2023

@bgamari I'm trying to look into this now, but I'm unable to reproduce it. Would you mind sharing the relevant parts of your configuration, and maybe open a new issue?

@bgamari
Copy link
Contributor

bgamari commented Jan 12, 2024

Strangely I now seem unable to reproduce as well after having upgraded to the current state of nixos-23.11 as of today.

@bgamari
Copy link
Contributor

bgamari commented Jan 12, 2024

Actually, I take it back (the server is up, but all of the content is missing). I have opened #280588 to track this.

AndrewKvalheim added a commit to AndrewKvalheim/nixpkgs that referenced this pull request Sep 19, 2024
nartsisss pushed a commit to nartsisss/nixpkgs that referenced this pull request Sep 20, 2024
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.

9 participants