wordpress: replace the dbPassword option with dbPasswordFile #24146
Conversation
|
@basvandijk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @qknight, @grahamc and @ehmry to be potential reviewers. |
|
I'd prefer to have both options. |
|
@globin I prefer to make users conscious about the safety of their keys. An option like The Of course the old unsafe behaviour is easy to recreate using |
basvandijk
force-pushed
the
wordpress-dbPasswordFile
branch
from
b5f1ffc
to
bc52e9b
Compare
|
@globin I changed my mind and followed your preference. The nice thing is that this is also backwards compatible. |
basvandijk
force-pushed
the
wordpress-dbPasswordFile
branch
from
bc52e9b
to
cc191ef
Compare
|
It's possible to add |
|
btw, as is, this one is a breaking change |
|
@RonnyPfannschmidt what breaks because of this change? |
|
@basvandijk any user that has this already configured afterwards has a broken configuration, because the old thing is gone, there is not the slightest hint of a transition help either |
|
@RonnyPfannschmidt the old thing isn't gone. The change should be backwards compatible. |
|
indeed, i am sorry, i missed the |
| example = "wordpress"; | ||
| }; | ||
| dbPasswordFile = mkOption { | ||
| type = types.str; | ||
| default = toString (pkgs.writeTextFile { |
There was a problem hiding this comment.
I'm not sure, but I think more complex defaults like this are usually moved to the implementation with a mkDefault
There was a problem hiding this comment.
Correct, otherwise the manual is rebuilt when dbPassword changes in your config.
There was a problem hiding this comment.
Also, where's the config attrset for this module?
There was a problem hiding this comment.
@globin note that wordpress is not mentioned in the manual.
@LnL7 wordpress.nix is not a traditional NixOS module and doesn't have a config attribute. It should be used as:
{
services.httpd.extraSubservices = [
{ serviceType = "wordpress";
dbName = "...";
themes = [];
plugins = [];
}
];
}So there's no need and no place to use mkDefault.
There was a problem hiding this comment.
basvandijk
force-pushed
the
wordpress-dbPasswordFile
branch
from
cc191ef
to
29f5d7f
Compare
We shouldn't force users to store passwords in the world-readable Nix store.
basvandijk
force-pushed
the
wordpress-dbPasswordFile
branch
from
29f5d7f
to
d6c2c3c
Compare
|
Similarly to other web-apps I added an example to the Is there anything that prevents this being mergeable? |
|
love this concept, we should not stop at password(s) though. same is true for owncloud/nextcloud and so on. |
|
@qknight thanks and I agree that |
|
@qknight thanks for the merge! Can this also be cherry-picked on |
…4146) We shouldn't force users to store passwords in the world-readable Nix store.
|
Hi @qknight, it would be great if you can cherry pick this on |
|
@basvandijk added it to |
We shouldn't force users to store passwords in the world-readable Nix store so this commit replaces the
dbPasswordoption withdbPasswordFilewhich should be set to a file that contains the wordpress DB password. Users can then provision this file however they want, either by storing it unsafely in the Nix store usingpkgs.writeTextFileor safely using something like nixops key management.It would be great if this can also be cherry-picked on
release-17.03.