nixos/cfdyndns: add option to use CF token #253728
Conversation
nrdxp
force-pushed
the
cfdyndns-token
branch
2 times, most recently
from
766c4ad
to
a010adc
Compare
nrdxp
force-pushed
the
cfdyndns-token
branch
from
a010adc
to
7e2d28c
Compare
| CLOUDFLARE_RECORDS="${concatStringsSep "," cfg.records}"; | ||
| }; | ||
| script = '' | ||
| ${optionalString (cfg.apikeyFile != null) '' | ||
| export CLOUDFLARE_APIKEY="$(cat ${escapeShellArg cfg.apikeyFile})" | ||
| export CLOUDFLARE_EMAIL="${cfg.email}" | ||
| ''} | ||
| ${optionalString (cfg.apiTokenFile != null) '' |
There was a problem hiding this comment.
What will be used if both apikeyFile and apiTokenFile are set? Should we add an assertion to make sure only one of them is set to reduce confusing?
There was a problem hiding this comment.
Well it depends on the implementation. In the current package in nixpkgs, the Rust binary prefers the api key, email combo, and will use it if both are present. In my recent refactor, the opposite is true and the token is prefered.
Either way, it won't hurt anything and nothing will break, but perhaps an assert would be a nicety; by no means essential though.
There was a problem hiding this comment.
This isn't quite true, it will prefer the APITOKEN: https://github.com/colemickens/cfdyndns/blob/31d576a3700989fe7533081b238154289eab84e2/src/main.rs#L42-L55
There was a problem hiding this comment.
ah did you change that recently or am I just going senile 😅
There was a problem hiding this comment.
I am jetlagged, and I don't want to call you senile... but... according to the blame, it was at least 2 years ago 😄 https://github.com/colemickens/cfdyndns/blame/31d576a3700989fe7533081b238154289eab84e2/src/main.rs#L42-L55
There was a problem hiding this comment.
I think my question is, why did I implement it in cfdyndns and then never make it usable in the module here? Who knows....
|
LGTM, I'll merge soon unless someone else protests and thinks the assert is necessary. |
|
hey, it looked like that PR was ready to merge, so I went ahead, I hope it's fine by everyone. |
|
also small thing I just noticed looking at this module. Perhaps we should run it as a DynamicUser ? |
Description of changes
Adds an option to set a file path containing a cloudflare API token, instead of using an api key/email combo
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)