transfer-sh: init at 1.6.0 #239916
transfer-sh: init at 1.6.0 #239916
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
pinpox
requested review from
mweinelt,
AndersonTorres,
SebTM and
Lassulus
and removed request for
mweinelt and
AndersonTorres
SuperSandro2000
changed the title
|
The commit messages do not fit CONTRIBUTING.md. |
There was a problem hiding this comment.
Please follow the contributing guide when naming your commits.
Could this module use freeform settings instead?
| type = types.string; | ||
| }; | ||
|
|
||
| LISTENER = mkOption { |
There was a problem hiding this comment.
LISTENER is not a great name for the port option
There was a problem hiding this comment.
That is true, but I sticked to the upstream namings to make it easy to look up the docs. Unless you have a strong opinion against it, I'd just stick with the name to be consistent with upstream docs
| AWS_SECRET_KEY = mkOption { | ||
| description = "aws access key"; | ||
| default = null; | ||
| type = types.nullOr types.str; | ||
| }; |
There was a problem hiding this comment.
This promotes setting configuration in an insecure way
There was a problem hiding this comment.
Should I take it out? I added a warning further down
| type = types.nullOr types.int; | ||
| }; | ||
|
|
||
| PROFILE_LISTENER = mkOption { |
There was a problem hiding this comment.
The settings is called like this upstream. I'd vouch for keeping it as is so the docs are consistent.
There was a problem hiding this comment.
This is irrelevant.
No one here calls an attribute --enable-simd-specific-optimizations just "for keeping it as is so the docs are consistent" with Autoconf-like scripts, or -DENABLE_SIMD_SPECIFIC_OPTIMIZATIONS just "for keeping it as is so the docs are consistent" with CMake scripts.
Just look at any package.nix | default.nix elsewhere.
On the other hand, the usage you are doing around the code is a repeating sequence of things like
PROFILE_LISTENER = toString cfg.PROFILE_LISTENER;and no one will die if you use
PROFILE_LISTENER = toString cfg.profileListener;
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
and removed
10.rebuild-darwin: 0
Noted. I'll squash when everything else is done. Should module and pkg be separate commits or can I "init at xxx" both at once? |
|
Should match the format now. |
|
can we maybe put all the CAPSLOCK option inside a freeform submodule? and just document the couple ones which are important? |
| HTTP_AUTH_IP_WHITELIST = mkOption { | ||
| description = "comma separated list of ips allowed to upload without being challenged an http auth"; | ||
| default = [ ]; | ||
| type = with types; listOf str; |
There was a problem hiding this comment.
I'd prefer uses of with like this are also removed but its much less important than the top most one and is a preference thing.
| GA_KEY = mkOption { | ||
| description = "google analytics key for the front end"; | ||
| default = null; | ||
| type = types.nullOr types.str; |
There was a problem hiding this comment.
plain text secrets are never fun. If an alternative way to get the secret to the service exists you should certainly be using that, I'd say this should be removed if there is any alternative.
|
Should I close this PR in favor of #283660 ? I don't mind and think it makes sense to decide on one of the two PRs and focus work there to push it over the finish line. |
|
The other PR got merged instead. Please see if there are any missing bits. |
Description of changes
Added a package and module for transfer.sh, addressing #239627
I'm sure there are things that can be done in a cleaner way, please let me know and I'll iterate over the code until it fits the quality standards.
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/)