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/dokuwiki: add support for multi-site, additional plugins and templates #83769

Merged
merged 9 commits into from Apr 20, 2020

Conversation

dadada
Copy link
Contributor

@dadada dadada commented Mar 30, 2020

Motivation for this change

This MR introduces multi-site dokuwiki configurations (e.g. multiple nginx servers) using the <name?> option syntax. It also enables the use of additional plugins and templates similarly to how wordpress.nix does it.

So far everything seems to work fine, but marking as WIP since I'd like to do some manual testing first. I can split this up into two MRs if requested.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dadada
Copy link
Contributor Author

dadada commented Mar 30, 2020

Pinging @1000101 maintainer.

@dadada dadada changed the title WIP: Nixos/dokuwiki multi-site support and additional plugins and templates WIP: dokuwiki: multi-site support and additional plugins and templates Mar 30, 2020
@dadada dadada force-pushed the nixos/dokuwiki-multi-server branch from 371e4af to 1d34444 Compare March 30, 2020 12:07
@dadada dadada changed the title WIP: dokuwiki: multi-site support and additional plugins and templates WIP: dokuwiki: add support for support multi-site, additional plugins and templates Mar 30, 2020
@1000101
Copy link
Member

1000101 commented Mar 30, 2020

@dadada thanks a million for picking up on this, I'll give it a go in a couple of hours.

@aanderse might want to have a look at this, too!

@dadada dadada force-pushed the nixos/dokuwiki-multi-server branch 4 times, most recently from 54d7ca8 to efe4248 Compare March 30, 2020 20:28
@1000101
Copy link
Member

1000101 commented Apr 1, 2020

@dadada Let me know when you're happy with the changes and I'll test it.

@dadada dadada changed the title WIP: dokuwiki: add support for support multi-site, additional plugins and templates dokuwiki: add support for support multi-site, additional plugins and templates Apr 4, 2020
@dadada
Copy link
Contributor Author

dadada commented Apr 4, 2020

Thanks for taking an interest. What's your opinion on the way the users.php is created? If I understand correctly, this was previously read-only / part of the derivation? Was there some specific reason for this? Should we put a warning if usersFile is defined?

@dadada
Copy link
Contributor Author

dadada commented Apr 4, 2020

I made two more minor changes. efc9258c97355b4ea6e313c7ab9447d894dbcf34 adds a dokuwiki user that owns all the state directories and files.

07504a1 changes the behaviour of usersFile and aclFile. Since they might contain private information, the contents should not end up in the nix store. If the options are defined, those files are generated at the given paths if they don't already exist.

If acl is specified, the contents of it end up in the nix store, but I consider this to be a valid use-case and included a warning in the documentation.

@dadada dadada changed the title dokuwiki: add support for support multi-site, additional plugins and templates dokuwiki: add support for multi-site, additional plugins and templates Apr 4, 2020
@dadada
Copy link
Contributor Author

dadada commented Apr 4, 2020

I'll need to do a quick reword of the commit messages that change nixos/dokuwiki

@dadada dadada force-pushed the nixos/dokuwiki-multi-server branch from 07504a1 to 5f2f935 Compare April 4, 2020 13:15
@dadada dadada changed the title dokuwiki: add support for multi-site, additional plugins and templates nixos/dokuwiki: add support for multi-site, additional plugins and templates Apr 4, 2020
@dadada
Copy link
Contributor Author

dadada commented Apr 4, 2020

@dadada Let me know when you're happy with the changes and I'll test it.

I'm happy, go ahead. :-)

@1000101
Copy link
Member

1000101 commented Apr 8, 2020

@GrahamcOfBorg test dokuwiki

@1000101
Copy link
Member

1000101 commented Apr 17, 2020

Thanks for taking an interest. What's your opinion on the way the users.php is created? If I understand correctly, this was previously read-only / part of the derivation? Was there some specific reason for this? Should we put a warning if usersFile is defined?

You mean users.auth.php (i.e. usersFile option)? It was actually not part of the derivation so that 1. credentials would not end up in nix store 2. it would be mutable (users could change their passwords).

Generally, I was trying to have as much options as I can immutable, with obvious ones mutable (https://github.com/NixOS/nixpkgs/pull/77830/files#r368233914 (resolved conversation) ).

@dadada
Copy link
Contributor Author

dadada commented Apr 17, 2020

You mean users.auth.php (i.e. usersFile option)? It was actually not part of the derivation so that 1. credentials would not end up in nix store 2. it would be mutable (users could change their passwords).

Ok, I might have misunderstood how types.path works. I was under the impression that the path will automatically be copied to the derivation / package, but this does not seem to be the case.

Generally, I was trying to have as much options as I can immutable, with obvious ones mutable (https://github.com/NixOS/nixpkgs/pull/77830/files#r368233914 (resolved conversation) ).

I made some changes so that the mutable files for usersFile and aclFile will not be automatically created unless specified, which should be identical to how these parameters behaved in your version.

Copy link
Member

@1000101 1000101 left a comment

Choose a reason for hiding this comment

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

Other than that, works really great, awesome job!!!

nixos/modules/services/web-apps/dokuwiki.nix Outdated Show resolved Hide resolved
{
# Enable encryption by default,
options.forceSSL.default = true;
options.enableACME.default = true;
Copy link
Member

@1000101 1000101 Apr 17, 2020

Choose a reason for hiding this comment

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

I know I have introduced this option, but on a second thought, it is quite unnecessary to force these settings upon user. Wouldn't it be reasonable to drop it - along with forceSSL?

But in that case, I guess: fastcgi_param HTTPS on; would have to go, too.

I'm open to discussion on this one :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it would be wise to go with the default that works in most cases. ACME does not always work depending on restrictive DNS CA records or missing internet connection.

The default seems to be to disable ACME and not to force SSL. But it should be enabled in the example.

Regarding ninx fastcgi, if_not_empty should do the trick.

If the directive is specified with if_not_empty (1.1.11) then such a parameter will be passed to the server only if its value is not empty:

fastcgi_param HTTPS $https if_not_empty;

Copy link
Member

Choose a reason for hiding this comment

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

Sounds about right, let's do it that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since would be an independent change, so you could open a new MR for this.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, could do that, but since you did such a great job in refactoring a few things here and there, I though it would be a nice addition to the PR. But you're right, we can definitely keep it separated, it's just nice to have and not subject of this PR.

@1000101
Copy link
Member

1000101 commented Apr 17, 2020

You mean users.auth.php (i.e. usersFile option)? It was actually not part of the derivation so that 1. credentials would not end up in nix store 2. it would be mutable (users could change their passwords).

Ok, I might have misunderstood how types.path works. I was under the impression that the path will automatically be copied to the derivation / package, but this does not seem to be the case.

Generally, I was trying to have as much options as I can immutable, with obvious ones mutable (https://github.com/NixOS/nixpkgs/pull/77830/files#r368233914 (resolved conversation) ).

I made some changes so that the mutable files for usersFile and aclFile will not be automatically created unless specified, which should be identical to how these parameters behaved in your version.

Oh, I actually meant to say that I think your approach is correct (much better than mine, for that matter) as these files stay outside of /nix/store (in datadir) and they don't need to be created by hand. I'm sorry I didn't make myself clear!!! I think you're right the whole time, or am I missing something? :)))

@dadada
Copy link
Contributor Author

dadada commented Apr 17, 2020

Oh, I actually meant to say that I think your approach is correct (much better than mine, for that matter) as these files stay outside of /nix/store (in datadir) and they don't need to be created by hand. I'm sorry I didn't make myself clear!!! I think you're right the whole time, or am I missing something? :)))

Thanks, but there might be two aspects here:

  1. private data is not stored in the nix store
  2. the configuration is valid (the paths for usersFile and aclFile exist)

  1. With your approach usersFile and aclFile already were outside of /nix/store if specified, so everything should be fine. Otherwise they would be immutable, so no private data would end up in the nix store. Unless you consider the contents of acl to be private, hence the added warning in the description.

  2. Since usersFile and aclFile are both types.path, they are checked at configuratio time and if they don't exists, nixos-reconfigure fails, which is good. So no problem here either. The same would happen with the changes I made (still types.path).

I think the best would be to not create them, if usersFile or aclFile was not specified, but make them both types.str, so building the configuration does not fail, but the files are created if they don't exist at service startup.

@dadada
Copy link
Contributor Author

dadada commented Apr 17, 2020

I think the best would be to not create them, if usersFile or aclFile was not specified, but make them both types.str, so building the configuration does not fail, but the files are created if they don't exist at service startup.

On the other hand: If the file can't be created or has the wrong permissions, the service will fail at startup, which makes this kind-of dependent on the system and not 100% reproducible. I think the wireguard module does a similar thing with types.str and the private key.

@1000101
Copy link
Member

1000101 commented Apr 17, 2020

I think the best would be to not create them, if usersFile or aclFile was not specified, but make them both types.str, so building the configuration does not fail, but the files are created if they don't exist at service startup.

On the other hand: If the file can't be created or has the wrong permissions, the service will fail at startup, which makes this kind-of dependent on the system and not 100% reproducible. I think the wireguard module does a similar thing with types.str and the private key.

Sure, but what about leaving default value of usersFile and aclFile set to datadir, i.e. /var/lib/dokuwiki/${name} ant not null (as you had in previous commits)? Would that hurt anything? I think it's quite neat that it kind of works right out of the box, but the user can still change it if needed.

@dadada
Copy link
Contributor Author

dadada commented Apr 17, 2020

I think the best would be to not create them, if usersFile or aclFile was not specified, but make them both types.str, so building the configuration does not fail, but the files are created if they don't exist at service startup.

On the other hand: If the file can't be created or has the wrong permissions, the service will fail at startup, which makes this kind-of dependent on the system and not 100% reproducible. I think the wireguard module does a similar thing with types.str and the private key.

Sure, but what about leaving default value of usersFile and aclFile set to datadir, i.e. /var/lib/dokuwiki/${name} ant not null (as you had in previous commits)? Would that hurt anything? I think it's quite neat that it kind of works right out of the box, but the user can still change it if needed.

I thought about it, but that would mean creating aclFile even if acl is used. So changes to aclFile would not affect anything. Same if aclUse is false, there would still be a usersFile.

Copy link
Contributor Author

@dadada dadada left a comment

Choose a reason for hiding this comment

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

How does one access aclUse inside of the function. The way I tried it does not seem to work. I don't get how the function is called / which arguments are passed to it.

@1000101
Copy link
Member

1000101 commented Apr 17, 2020

I think the best would be to not create them, if usersFile or aclFile was not specified, but make them both types.str, so building the configuration does not fail, but the files are created if they don't exist at service startup.

On the other hand: If the file can't be created or has the wrong permissions, the service will fail at startup, which makes this kind-of dependent on the system and not 100% reproducible. I think the wireguard module does a similar thing with types.str and the private key.

Sure, but what about leaving default value of usersFile and aclFile set to datadir, i.e. /var/lib/dokuwiki/${name} ant not null (as you had in previous commits)? Would that hurt anything? I think it's quite neat that it kind of works right out of the box, but the user can still change it if needed.

I thought about it, but that would mean creating aclFile even if acl is used. So changes to aclFile would not affect anything. Same if aclUse is false, there would still be a usersFile.

Ok, let's leave it be, it's just a minor nitpicking anyway.

@dadada dadada force-pushed the nixos/dokuwiki-multi-server branch 3 times, most recently from 085f55d to 6ffb27b Compare April 18, 2020 09:41
@dadada
Copy link
Contributor Author

dadada commented Apr 18, 2020

Now aclUse = true; should be enough to create users.auth.php and acl.auth.php in the default locations.

@dadada dadada force-pushed the nixos/dokuwiki-multi-server branch from 6ffb27b to cfbc0c4 Compare April 18, 2020 09:50
@1000101
Copy link
Member

1000101 commented Apr 18, 2020

@GrahamcOfBorg test dokuwiki

@dadada dadada force-pushed the nixos/dokuwiki-multi-server branch from cfbc0c4 to adea2d1 Compare April 18, 2020 21:35
Enables multi-site configurations.

This break compatibility with prior configurations that expect options
for a single dokuwiki instance in `services.dokuwiki`.
Adds support for additional plugins and templates similarly to how
wordpress.nix does it.

Plugins and templates need to be packaged as in the example.
If usersFile is not set, a file is created along the stateDir that can
hold the users and supports dynamically adding users using the web GUI.
Use types.str instead of types.path to exclude private information from
the derivation.
Add a warinig about the contents of acl beeing included in the nix
store.
`aclFile` and `usersFile` will be set to a default value if `aclUse` is
specified and aclFile is not overriden by `acl`.
@dadada dadada force-pushed the nixos/dokuwiki-multi-server branch from adea2d1 to 2d86cca Compare April 18, 2020 21:37
@dadada
Copy link
Contributor Author

dadada commented Apr 18, 2020

Rebased changes onto master and squashed fixup commits.

@1000101
Copy link
Member

1000101 commented Apr 18, 2020

Rebased changes onto master and squashed fixup commits.

Oh, and you managed to find a way to access config values, great job! I'll test and review it again.

@1000101
Copy link
Member

1000101 commented Apr 18, 2020

@GrahamcOfBorg test dokuwiki

Copy link
Member

@1000101 1000101 left a comment

Choose a reason for hiding this comment

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

  • reviewed the diff and commit messages
  • made sure ofBorg build succeeded for all applicable platforms
  • ran nix-review without any failures
  • ran and tested the service

@1000101
Copy link
Member

1000101 commented Apr 19, 2020

@mmahut could you please help us check and merge if everything's fine by you?

@mmahut mmahut merged commit 60100a7 into NixOS:master Apr 20, 2020
devhell added a commit to openlab-aux/vuizvui that referenced this pull request Apr 24, 2020
With the merge of [1], the `.enable` check for
`config.services.dokuwiki` has been removed.

[1]: NixOS/nixpkgs#83769
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

3 participants