Fediversity/simple-nixos-fediverse
Archived
7
1
Fork 2
Code Issues 23 Pull requests 1 Actions Projects Wiki Activity

nixos-test-pixelfed-wip #22

Merged
taeer merged 11 commits from nixos-test-pixelfed-wip into main 2024-09-20 17:47:21 +02:00
Conversation 0 Commits 11 Files changed 13 +231 -94
taeer commented 2024-09-17 22:39:32 +02:00
Collaborator
Copy link

Making a PR for the rest of the work from the WIP branch (@Niols' refactor) so I can give a proper review with comments and such

Making a PR for the rest of the work from the WIP branch (@Niols' refactor) so I can give a proper review with comments and such
👍 1
taeer added 6 commits 2024-09-17 22:39:33 +02:00
taeer reviewed 2024-09-17 23:19:40 +02:00
taeer left a comment
Author
Collaborator

Nice work! I think this cleans things up well. I left some comments about how nixos stuff works and tends to work.

Nice work! I think this cleans things up well. I left some comments about how nixos stuff works and tends to work.
fediversity/default.nix Outdated
@ -0,0 +18,4 @@
enable = mkOption {
type = types.bool;
default = false;
};
taeer commented 2024-09-17 22:44:05 +02:00
Author
Collaborator

This can just be

fediversity = mkEnableOption "the collection of services bundled under fediversity";
This can just be ``` fediversity = mkEnableOption "the collection of services bundled under fediversity"; ```
Niols commented 2024-09-20 16:34:32 +02:00
Owner

Thanks! Done in 2ff8975b6b.

Thanks! Done in 2ff8975b6b2f19d5ffbd7574bd76494bf532d048.
Niols marked this conversation as resolved
fediversity/default.nix Outdated
@ -0,0 +21,4 @@
};
garage = mkOption {
type = types.anything;
taeer commented 2024-09-17 22:49:35 +02:00
Author
Collaborator

You almost never need types.anything. And certainly in this case the set of sub-options should be defined. This can be done with simple attrsets

garage = {
  api = {
    url = mkOption {
      ...
    };
  };
};

Also

We don't need to have a very clear sense of which options belong where yet, but eventually I think the top-level fediversity options should be the ones that should be set publicly (via NixPanel), and probably the ports being used in garage shouldn't be among them.

I'm not sure what the right namespace to use for those options is... maybe fediversity-private or fediversity.private...

You almost never need `types.anything`. And certainly in this case the set of sub-options should be defined. This can be done with simple attrsets ``` garage = { api = { url = mkOption { ... }; }; }; ``` --- Also We don't need to have a very clear sense of which options belong where **yet**, but eventually I think the top-level `fediversity` options should be the ones that should be set publicly (via NixPanel), and probably the ports being used in garage shouldn't be among them. I'm not sure what the right namespace to use for those options is... maybe `fediversity-private` or `fediversity.private`...
Niols commented 2024-09-20 17:16:23 +02:00
Owner

Done in 73939b9d87.

Done in 73939b9d8752ed4193ebae1b865c306d8eae4971.
Niols marked this conversation as resolved
fediversity/default.nix Outdated
@ -0,0 +25,4 @@
};
domain = mkOption {
type = types.string;
taeer commented 2024-09-17 22:53:40 +02:00
Author
Collaborator

should be types.str
types.string was being used for too many thing so it got deprecated and now there are several different string types. types.str is the one where you don't want to merge definitions if it's defined in more than one place

should be `types.str` `types.string` was being used for too many thing so it got deprecated and now there are several different string types. `types.str` is the one where you don't want to merge definitions if it's defined in more than one place
Niols commented 2024-09-20 16:36:10 +02:00
Owner

Done in d97772ccc4.

Done in d97772ccc4008417aec2117d62031e9123d8deba.
Niols marked this conversation as resolved
fediversity/default.nix Outdated
@ -0,0 +30,4 @@
mastodon.enable = mkOption { type = types.bool; default = false; };
pixelfed.enable = mkOption { type = types.bool; default = false; };
peertube.enable = mkOption { type = types.bool; default = false; };
taeer commented 2024-09-17 22:44:39 +02:00
Author
Collaborator

Similarly, mkEnableOption here

Similarly, `mkEnableOption` here
Niols marked this conversation as resolved
fediversity/default.nix Outdated
@ -0,0 +34,4 @@
};
};
config.fediversity = {
taeer commented 2024-09-17 23:05:23 +02:00
Author
Collaborator

This should probably be under a call to lib.mkDefault. Or simply put behind the default argument of mkOption. It seems to be the custom (where possible---sometimes it's not) to put it under the default argument.

This should probably be under a call to `lib.mkDefault`. Or simply put behind the `default` argument of `mkOption`. It seems to be the custom (where possible---sometimes it's not) to put it under the `default` argument.
Niols commented 2024-09-20 17:16:33 +02:00
Owner

Done in 73939b9d87.

Done in 73939b9d8752ed4193ebae1b865c306d8eae4971.
Niols marked this conversation as resolved
fediversity/default.nix Outdated
@ -0,0 +56,4 @@
pixelfed.domain = "pixelfed.${config.fediversity.domain}";
mastodon.domain = "mastdodon.${config.fediversity.domain}";
peertube.domain = "peertube.${config.fediversity.domain}";
taeer commented 2024-09-17 23:00:52 +02:00
Author
Collaborator

there's a style decision here between

  1. defining our own top-level e.g. fediversity.pixelfed.domain, and
  2. referring to e.g. services.pixelfed.domain everywhere.

(1) has the advantage of grouping everything in one place and under one naming scheme, as opposed to needing to know that e.g. mastodon's is under services.mastodon.localDomain instead.
It has the disadvantage of having basically a duplicate option, that really should always be exactly identical to another option. Now if someone sets services.pixelfed.domain somewhere, it won't get propagated to all the other places that use it, because they refer to fediversity.pixelfed.domain. It's also hard to know where to stop with option (1), since you could recategorize all the options under some different scheme at the top level, which might be somewhat more consistent.

I tend to go with option (2), but in a tightly controlled module like we're planning, I don't think it's objectively the right choice, just a style difference as I said.

In the case of the garage options above, we sort of have the same question. But in that case there's information we can't easily get out of the options that are available in services.garage (e.g. just the port itself), so I think it does make sense to define our own options.

Though we should probably discuss with the maintainer of the nixpkgs module if it makes sense to upstream those changes.

there's a style decision here between 1. defining our own top-level e.g. `fediversity.pixelfed.domain`, and 2. referring to e.g. `services.pixelfed.domain` everywhere. (1) has the advantage of grouping everything in one place and under one naming scheme, as opposed to needing to know that e.g. mastodon's is under `services.mastodon.localDomain` instead. It has the disadvantage of having basically a duplicate option, that really should always be exactly identical to another option. Now if someone sets `services.pixelfed.domain` somewhere, it won't get propagated to all the other places that use it, because they refer to `fediversity.pixelfed.domain`. It's also hard to know where to stop with option (1), since you could recategorize all the options under some different scheme at the top level, which might be somewhat more consistent. I tend to go with option (2), but in a tightly controlled module like we're planning, I don't think it's objectively the right choice, just a style difference as I said. --- In the case of the `garage` options above, we sort of have the same question. But in that case there's information we can't easily get out of the options that are available in `services.garage` (e.g. just the port itself), so I think it does make sense to define our own options. Though we should probably discuss with the maintainer of the nixpkgs module if it makes sense to upstream those changes.
taeer commented 2024-09-17 23:01:22 +02:00
Author
Collaborator

Though if you are going to define fediversity.pixelfed.domain, it does have to be declared as an option somewhere 😆

Though if you are going to define `fediversity.pixelfed.domain`, it does have to be declared as an option somewhere 😆
tests/mastodon-garage.nix
@ -96,3 +96,4 @@
with subtest("access garage"):
## REVIEW: could we grab `config.fediversity.garage.api.url` here in some way?
server.succeed("mc alias set garage http://s3.garage.localhost:3900 --api s3v4 --path off $AWS_ACCESS_KEY_ID $AWS_SECRET_ACCESS_KEY")
taeer commented 2024-09-17 23:15:39 +02:00
Author
Collaborator

Yes! In typical nix dynamically-typed fashion, testScript can also be a function! And we can have

testScript = { nodes, ... }: ''
...
   ${nodes.server.fediversity.garage.api.url}
...
'';
Yes! In typical nix dynamically-typed fashion, `testScript` can also be a function! And we can have ``` testScript = { nodes, ... }: '' ... ${nodes.server.fediversity.garage.api.url} ... ''; ```
tests/mastodon-garage.nix Outdated
@ -121,6 +122,7 @@ pkgs.nixosTest {
raise Exception("mastodon did not send a content security policy header")
csp = csp_match.group(1)
# the img-src content security policy should include the garage server
## REVIEW: could we grab `config.fediversity.garage.web.url` here in some way?
taeer commented 2024-09-17 23:16:01 +02:00
Author
Collaborator

Yes, though we need to be careful about escaping for the regex

Yes, though we need to be careful about escaping for the regex
Niols added 1 commit 2024-09-20 16:34:27 +02:00
Niols added 1 commit 2024-09-20 16:36:04 +02:00 
`types.string` was being used for too many thing so it got deprecated
and now there are several different string types. `types.str` is the one
where you don't want to merge definitions if it's defined in more than
one place

Co-authored-by: Taeer Bar-Yam <taeer.bar-yam@moduscreate.com>
Niols added 1 commit 2024-09-20 17:14:57 +02:00 
- make things such as `fediversity.garage.api.port` into actual options
  with the right default value
- move them under `fediversity.internal`

Co-authored-by: Taeer Bar-Yam <taeer.bar-yam@moduscreate.com>
Niols added 1 commit 2024-09-20 17:20:58 +02:00
Niols added 1 commit 2024-09-20 17:46:21 +02:00
taeer merged commit fa0a01f868 into main 2024-09-20 17:47:21 +02:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Fediversity/simple-nixos-fediverse#22
No description provided.
Delete branch "nixos-test-pixelfed-wip"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?