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/*: Add types to the database module options #76667

Closed
wants to merge 6 commits into from

Conversation

@dasJ
Copy link
Member

dasJ commented Dec 29, 2019

Add types to all database module options. See each commit for easier reviewing.

See also #76184

Some pings (sorry for bothering you):

  • redis: @Mic92
  • virtuoso: nobody (is anyone even using the module?)
  • firebird: nobody
  • clickhouse: @Ma27
  • mongodb: @phile314
  • memcached: @dasJ (oh that's me)
@dasJ dasJ changed the title Types1 nixos/*: Add types to the database module options Dec 29, 2019
@dasJ dasJ mentioned this pull request Dec 29, 2019
6 of 361 tasks complete
@dasJ
Copy link
Member Author

dasJ commented Dec 29, 2019

@GrahamcOfBorg test memcached mongodb clickhouse firebird virtuoso redis

@dasJ dasJ force-pushed the helsinki-systems:types1 branch from 42d58b5 to 815eaf5 Dec 29, 2019
@Ma27
Ma27 approved these changes Dec 30, 2019
Copy link
Member

Ma27 left a comment

Thanks a lot for taking care of this, unfortunately I didn't manage to leave some useful feedback earlier - sorry! I left three (rather trivial) comments containing stuff I noticed, feel free to comment/resolve (if those are invalid :)).

@@ -64,20 +59,23 @@ in

port = mkOption {
default = "3050";

This comment has been minimized.

Copy link
@Ma27

Ma27 Dec 30, 2019

Member

Shouldn't we use an int here if that option now represents a port?

'';
};
enable = mkEnableOption ''
Whether to enable the Redis server. Note that the NixOS module for

This comment has been minimized.

Copy link
@Ma27

Ma27 Dec 30, 2019

Member

mkEnableOption's description is basically Whether to enable <name>, while <name> is the first argument passed to mkEnableOption. Thereforce I think that your text might seem rather confusing when looking at it from the manual. I'd suggest that you either adjust your text or that you do e.g. (mkEnableOption "opt-name") // { description = ''A little more details''; }.

@@ -16,45 +16,46 @@ in

options = {

services.memcached = {
services.memcached = with types; {

This comment has been minimized.

Copy link
@Ma27

Ma27 Dec 30, 2019

Member

(nitpick, probably irrelevant) I'm not sure if we actually benefit from adding even more with ...; expressions. While I think that those are pretty handy and convenient, I know that especially beginners will have a hard time understanding where a variable is actually coming from if it was passed through several expressions using with ...;.

Copy link
Contributor

phile314 left a comment

Only looked at the mongodb changes, but those look good to me.

@stale
Copy link

stale bot commented Jun 27, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.
@stale stale bot added the 2.status: stale label Jun 27, 2020
@Mic92 Mic92 mentioned this pull request Jun 30, 2020
0 of 10 tasks complete
@Mic92
Copy link
Contributor

Mic92 commented Jun 30, 2020

I rebased it here: #91813

@Mic92 Mic92 closed this Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.