mcaptcha package and module with nixos tests #61

jfly · 2023-09-26T07:50:05Z

This closes #17

pkgs/mcaptcha/default.nix

lorenzleutgeb · 2023-09-26T08:30:20Z

CI is failing because you seem to be introducing IFD.

jfly · 2023-09-27T08:20:02Z

@lorenzleutgeb, we've updated this PR to remove IFD. Unfortunately, this required vendoring a number of Rust Cargo.lock files, and a couple of node package.json files. As far as we can tell, this is just the way things have to be:

buildRustPackage can work without vendoring Cargo.lock, but it doesn't support git dependencies, which unfortunately are used in the mCaptcha ecosystem.
buildYarnPackage seems to require you to vendor the package.json. We weren't able to find a way around that.

lorenzleutgeb

Regarding the module, you could follow Nix RFC 42 and split between "regular options" and "settings", where the settings attribute that takes "settings" is a freeform type. In the PR against nixpkgs for Rosenpass, this came up and I think it's a better solution. See also https://github.com/NixOS/nixpkgs/blob/a06625089d41f3cf926ac0f21026bf0a0fd9b74f/nixos/modules/services/networking/rosenpass.nix

modules/mcaptcha.nix

pkgs/mcaptcha/default.nix

pkgs/mcaptcha-cache/default.nix

matusf · 2023-09-29T08:08:54Z

Hi @lorenzleutgeb, we have split the config between "regular options" and "settings" as specified in RFC 42. Could you please take another look on this PR? Thanks!

mightyiam · 2023-10-02T05:14:02Z

@lorenzleutgeb if you intend to review this, we'll wait for you. Just let us know please. No pressure.

lorenzleutgeb · 2023-10-02T06:06:33Z

@lorenzleutgeb if you intend to review this, we'll wait for you. Just let us know please. No pressure.

Yes, I do intend. Thanks for the reminder and sorry for the delay.

modules/mcaptcha.nix

lorenzleutgeb · 2023-10-02T05:56:58Z

modules/mcaptcha.nix

+
+      options.database.name = mkOption {
+        type = types.str;
+        description = "Applies both when `database.createLocally` is set and not.";


Suggested change

description = "Applies both when `database.createLocally` is set and not.";

description = "Applies both when {option}`database.createLocally` is set and not.";

Use option literals (other occurrences below).

I would also encourage you to spell out the whole path to the option, or for a sibling option to not specify any parent at all. Relative references within options are tricky to understand/parse.

Suggested change

description = "Applies both when `database.createLocally` is set and not.";

description = "Applies both when {option}`services.mcaptcha.settings.database.createLocally` is set and not.";

Possibly even use the option itself, which is easier to refactor:

Suggested change

description = "Applies both when `database.createLocally` is set and not.";

description = "Applies both when {option}`${opt.settings.database.createLocally}` is set and not.";

where opt = options.services.mcaptcha (usually defined next to cfg).

Edit: Turns out I got tricked here, as I only discovered services.flarum database.createLocally later on. Another indication towards specifying the full path.

Unresolved (added further comments to make this clearer).

We believe this is now resolved.

lorenzleutgeb · 2023-10-02T06:01:18Z

modules/mcaptcha.nix

+
+    description = ''
+      Extra settings. Best sources of documentation for settings seem to be
+      https://github.com/mCaptcha/mCaptcha/blob/master/config/default.toml


Suggested change

https://github.com/mCaptcha/mCaptcha/blob/master/config/default.toml

- <https://github.com/mCaptcha/mCaptcha/blob/master/config/default.toml>

Maybe don't link to the master branch, but to the revision that you read, probably a tag related to a release. Options might of course change as there are new versions released, but the file might also disappear or move. It's a trade-off.

Unresolved.

We decided to simply not provide a URL.

lorenzleutgeb · 2023-10-03T14:14:06Z

modules/mcaptcha.nix

+    type = lib.types.submodule {
+      freeformType = settingsFormat.type;
+
+      options.database.name = mkOption {


Consider moving all these options into an attribute set:

options = { database.name = mkOption { /* ... */ }; database.username = mkOption { /* ... */ }; /* ... */ };

Unresolved.

Personally, I like this Nix language feature and use it pretty much wherever I can. I bet if more tried it more would like it, as well. It's there for a reason.

I like two aspects of this feature:

In cases of long literal attribute sets it makes the full path of the value clear from any line. No scrolling required.

Text diffing (what we have in git) is more agreeable with this.
For example, changing from

{ foo.bar = 2; };

Could be either:

{ foo = { bar = 2; baz = 3; } }

Or:

{ foo.bar = 2; foo.baz = 3; }

And so on.

Is this convincing?

modules/mcaptcha.nix

tests/mcaptcha/default.nix

lorenzleutgeb · 2023-10-03T14:38:03Z

tests/mcaptcha/default.nix

+    services.mcaptcha.enable = true;
+    services.mcaptcha.settings.server.port = port;
+    services.mcaptcha.settings.server.domain = "localhost";
+    services.mcaptcha.server.cookieSecretFile = pkgs.writeText "cookie-secret" "mcaptcha-cookie-secret-dm0tdGVzdC1ydW4tbWNhcHRjaGEtdGVzdHM";
+    services.mcaptcha.captcha.saltFile = pkgs.writeText "salt" "asdl;kjfhjawehfpa;osdkjasdvjaksndfpoanjdfainsdfaijdsfajlkjdsaf;ajsdfweroire";
+
+    services.mcaptcha.settings.database.name = "my_mcaptcha";
+    services.mcaptcha.settings.database.username = "role_mcaptcha";
+    services.mcaptcha.settings.database.hostname = "my_own_services";
+    services.mcaptcha.settings.database.port = 5432;
+    services.mcaptcha.database.passwordFile = pkgs.writeText "db-password" "mcaptcha-db-secret";
+
+    services.mcaptcha.redis.passwordFile = pkgs.writeText "redis-secret" "(*&(*):ps@r}";
+    services.mcaptcha.redis.host = "my_own_services";


Again, probably a matter of taste, but I don't like giving the full path to each option. My eyes have to work way too much. I prefer attribute sets.

Sure, we don't mind, but there are so many of these we don't feel it's worth our time. How does that sound?

You select the whole block (something like va{, or Vjjjj... in Vim), and then you replace (something like :%s/\s+services\.mcaptcha// in Vim). Then you ensure that you have braces around the block. Then you format the file using alejandra. That takes less than a minute. If you have good LSP integration, then merging such lines into an attribute set is even easier. Also, you have introduced these lines at some point. When you introduce the third or fourth line with this much redundancy, then it'd be a good time to think about how to get rid of it right there and then.

Consider resolving in favor of our previous comment.

lorenzleutgeb · 2023-10-03T14:39:49Z

tests/mcaptcha/default.nix

+    services.postgresql.enable = true;
+    services.postgresql.enableTCPIP = true;
+    services.postgresql.initialScript = pkgs.writeText "postgresql-init-script" ''
+      CREATE ROLE role_mcaptcha WITH LOGIN PASSWORD 'mcaptcha-db-secret';
+      CREATE DATABASE my_mcaptcha;
+      GRANT ALL PRIVILEGES ON DATABASE my_mcaptcha TO role_mcaptcha;
+    '';
+    services.postgresql.authentication = ''
+      #type  database  DBuser         auth-method
+      host   all       all 0.0.0.0/0  md5
+    '';
+    services.redis.servers.mcaptcha.enable = true;
+    services.redis.servers.mcaptcha.port = 6379;
+    services.redis.servers.mcaptcha.bind = null;
+    services.redis.servers.mcaptcha.extraParams = [
+      "--loadmodule"
+      "${pkgs.mcaptcha-cache}/lib/libcache.so"
+    ];
+    services.redis.servers.mcaptcha.requirePass = "(*&(*):ps@r}";


I'd refactor this into:

services = { postgresql = { enable = true; /* ... */ }; redis.servers.mcaptcha = { enable = true; /* ... */ }; };

Is there a policy? Should we simply accept your suggestion? I don't mind.

No, there is not. For now, it is a matter of taste. I just could not resist commenting.

Please respond. If you want to ignore my comment, please comment that you are doing so.

Consider resolving in favor of our previous comment.

tests/mcaptcha/default.nix

mightyiam · 2023-10-20T08:33:27Z

@lorenzleutgeb the way we at Fungus left this is that we feel your feedback items have been addressed. Would you like to confirm?

lorenzleutgeb · 2023-10-20T09:43:38Z

There are many "unresolved conversations". I'd hope that you would comment why you decide not to resolve them, or if you add changes that comply with my comment, mark them as resolved. I'll do that now.

modules/mcaptcha.nix

pkgs/by-name/mcaptcha/package.nix

lorenzleutgeb · 2023-10-20T09:56:11Z

modules/mcaptcha.nix

+
+      options.database.name = mkOption {
+        type = types.str;
+        description = "Applies both when `database.createLocally` is set and not.";


Unresolved (added further comments to make this clearer).

lorenzleutgeb

@mightyiam I did another pass. Also pressed "resolved" on some earlier conversations, and commented once more on others that are unresolved.

modules/mcaptcha.nix

lorenzleutgeb · 2023-10-23T09:16:24Z

If there are open conversations, please either respond to them or mark them as resolved before requesting another review.

Conversation that are missing your reply even though you requested another review:

Conversations that were resolved, but you did not indicate that you resolved them:

lorenzleutgeb · 2023-10-23T09:47:08Z

I have politely asked multiple times for you to reply to unresolved conversations and mark conversations that you have resolved as resolved. You just keep hitting "request review" without any further comment. This is annoying. I will not review this anymore before you at least reply to the open conversations.

If you don't care about resolving these things, then you might as well merge yourself.

augustebaum · 2023-10-23T13:58:59Z

I have politely asked multiple times for you to reply to unresolved conversations and mark conversations that you have resolved as resolved. You just keep hitting "request review" without any further comment. This is annoying. I will not review this anymore before you at least reply to the open conversations.

If you don't care about resolving these things, then you might as well merge yourself.

Oh no I'm sorry, it was definitely not our intention to make it seem like your recommendations were not taken into account! Personally I took the advice of @mightyiam that we shouldn't resolve conversations that we hadn't started, to be sure that we don't accidentally step on the reviewer's toes. We'll go over this in our next work session and resolve what was taken care of.

lorenzleutgeb · 2023-10-23T14:15:52Z

Oh no I'm sorry, it was definitely not our intention to make it seem like your recommendations were not taken into account! Personally I took the advice of @mightyiam that we shouldn't resolve conversations that we hadn't started, to be sure that we don't accidentally step on the reviewer's toes.

Okay. It seems we were/are just not on the same page on who should hit "resolved". Sorry about that. For trivial things where you just accept the reviewers suggestions as-is (like you did today) I think it's okay to mark the conversation as resolved yourself. Otherwise the reviewer will have to do that, and since it's so easy/trivial, it's just a bit of an annoyance. An additional pass over the changes just to realize that trivial things were done. This time is better spent on the non-trivial parts of the PR/review. When I PR something, I tend to keep the GitHub page open as I make these changes, and hit "resolved" right when I make the resolution.

For more complex changes, i.e., when it's not as obvious as "accept suggestion" or "don't accept suggestion", but rather a discussion, I do get the point about "stepping on the reviewer's toes".

For each iteration of review, I only got one bit of information: "please review". You did not tell me whether any points are still unresolved (in your opinion), so I must assume that you think that everything is resolved. So I go to the PR, check discussions, and there are obviously unresolved things. In such a scenario I like to get at least a comment what this will be addressed later, or a continued discussion in these conversations.

Note however, that I explicitly asked you to reply, which you didn't. And that's what got me grumpy.

We'll go over this in our next work session and resolve what was taken care of.

Thanks!

mightyiam · 2023-10-24T09:19:11Z

Missing comments is enough to cause mayhem. The specifics don't even matter. Sorry about that.

For more complex changes, i.e., when it's not as obvious as "accept suggestion" or "don't accept suggestion", but rather a discussion, I do get the point about "stepping on the reviewer's toes".

My concern with this approach is that there could be issues where the reviewer finds them non-trivial and they get resolved by the author thinking they are. Also, cases where the resolution would not appease the reviewer. Therefore I don't like pressing anyone's resolve button. This approach provides me with the guarantee that the reviewer is satisfied with each issue.

Is there anything remaining?

mightyiam · 2023-10-27T09:55:39Z

Rebased. @lorenzleutgeb .

closes #17 Co-authored-by: Shahar "Dawn" Or <mightyiampresence@gmail.com> Co-authored-by: Rohit <rohitsutradhar311@gmail.com> Co-authored-by: Matúš Ferech <matus.ferech@gmail.com> Co-authored-by: Alejandro Sanchez Medina <alejandrosanchzmedina@gmail.com> Co-authored-by: Auguste Baum <auguste.apple@gmail.com>

mightyiam · 2023-12-05T20:30:11Z

Congratulations! @realaravinth.

jfly requested a review from a team September 26, 2023 07:50

lorenzleutgeb reviewed Sep 26, 2023

View reviewed changes

pkgs/mcaptcha/default.nix Outdated Show resolved Hide resolved

pkgs/mcaptcha/default.nix Outdated Show resolved Hide resolved

jfly force-pushed the mCaptcha branch from deb0eb8 to adf9abb Compare September 27, 2023 08:14

jfly requested a review from lorenzleutgeb September 27, 2023 08:16

lorenzleutgeb requested changes Sep 27, 2023

View reviewed changes

jfly commented Sep 28, 2023

View reviewed changes

modules/mcaptcha.nix Outdated Show resolved Hide resolved

jfly commented Sep 28, 2023

View reviewed changes

pkgs/mcaptcha/default.nix Outdated Show resolved Hide resolved

jfly commented Sep 28, 2023

View reviewed changes

pkgs/mcaptcha-cache/default.nix Outdated Show resolved Hide resolved

matusf force-pushed the mCaptcha branch from adf9abb to 473c304 Compare September 29, 2023 07:45

jfly force-pushed the mCaptcha branch from 473c304 to 011c08e Compare September 29, 2023 08:00

matusf requested a review from lorenzleutgeb September 29, 2023 08:09

lorenzleutgeb requested changes Oct 3, 2023

View reviewed changes

matusf force-pushed the mCaptcha branch from eea1d3c to 9a3f6e5 Compare October 5, 2023 06:38

mightyiam requested a review from lorenzleutgeb October 5, 2023 06:41

mightyiam force-pushed the mCaptcha branch from 9a3f6e5 to 72b4e9f Compare October 10, 2023 05:23

mightyiam approved these changes Oct 20, 2023

View reviewed changes

lorenzleutgeb reviewed Oct 20, 2023

View reviewed changes

lorenzleutgeb requested changes Oct 20, 2023

View reviewed changes

augustebaum force-pushed the mCaptcha branch from 72b4e9f to 122f95e Compare October 23, 2023 07:37

mightyiam requested a review from lorenzleutgeb October 23, 2023 07:43

lorenzleutgeb requested changes Oct 23, 2023

View reviewed changes

modules/mcaptcha.nix Outdated Show resolved Hide resolved

modules/mcaptcha.nix Outdated Show resolved Hide resolved

modules/mcaptcha.nix Outdated Show resolved Hide resolved

modules/mcaptcha.nix Outdated Show resolved Hide resolved

augustebaum force-pushed the mCaptcha branch from 122f95e to 6851249 Compare October 23, 2023 09:41

augustebaum requested a review from lorenzleutgeb October 23, 2023 09:42

mightyiam force-pushed the mCaptcha branch from 6851249 to 4fdfd03 Compare October 24, 2023 09:10

mightyiam force-pushed the mCaptcha branch from 4fdfd03 to 9ca9dcb Compare October 27, 2023 08:46

mightyiam force-pushed the mCaptcha branch from bc0dc15 to 4c1cdbe Compare November 28, 2023 12:17

lorenzleutgeb merged commit 862baf6 into main Dec 5, 2023
2 checks passed

lorenzleutgeb deleted the mCaptcha branch December 5, 2023 20:15

	description = "Applies both when `database.createLocally` is set and not.";
	description = "Applies both when {option}`database.createLocally` is set and not.";

	https://github.com/mCaptcha/mCaptcha/blob/master/config/default.toml
	- <https://github.com/mCaptcha/mCaptcha/blob/master/config/default.toml>

mcaptcha package and module with nixos tests #61

mcaptcha package and module with nixos tests #61

Conversation

jfly commented Sep 26, 2023 • edited by matusf

lorenzleutgeb commented Sep 26, 2023

jfly commented Sep 27, 2023

lorenzleutgeb left a comment

Choose a reason for hiding this comment

matusf commented Sep 29, 2023

mightyiam commented Oct 2, 2023

lorenzleutgeb commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzleutgeb Oct 20, 2023 • edited

Choose a reason for hiding this comment

augustebaum Oct 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorenzleutgeb Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

augustebaum Oct 24, 2023 • edited

Choose a reason for hiding this comment

mightyiam commented Oct 20, 2023

lorenzleutgeb commented Oct 20, 2023

Choose a reason for hiding this comment

lorenzleutgeb left a comment

Choose a reason for hiding this comment

lorenzleutgeb commented Oct 23, 2023

lorenzleutgeb commented Oct 23, 2023 • edited

augustebaum commented Oct 23, 2023

lorenzleutgeb commented Oct 23, 2023

mightyiam commented Oct 24, 2023

mightyiam commented Oct 27, 2023

mightyiam commented Dec 5, 2023

jfly commented Sep 26, 2023 •

edited by matusf

lorenzleutgeb Oct 20, 2023 •

edited

augustebaum Oct 24, 2023 •

edited

lorenzleutgeb Oct 5, 2023 •

edited

augustebaum Oct 24, 2023 •

edited

lorenzleutgeb commented Oct 23, 2023 •

edited