-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
Init Mobilizon #119132
Init Mobilizon #119132
Conversation
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/announcing-mixnix-build-elixir-projects-with-nix/2444/20 |
I'm okay to step in and become a maintainer if you want, let me know if I can do to help the two PRs to get merged faster. |
This is great work! I see what you mean with the problem around atom and strings. It does seem a little weird. I don't have a solution yet either. I do think what you have is enough to get things started. |
I just opened an issue upstream to see if they would be open to adding a runtime.exs |
@happysalada You reminded me that I wanted to talk about the I think Mobilizon's way of handling configuration since 1.1.0 is just straight up better than using But about having an upstream configuration which will fetch environment variables for configuration values, I just don't think it's feasible, unfortunately, there simply are to much configuration variables (it would mean a lot of work upstream to add an environment variable for everything, and work downstream to keep up to date). Also this poses some type issues, since the configuration is rich in type, but environment variables are only strings, complex configuration options would need to be parsed, and we kinda go back to the same problem. (For example, I don't see a simple way of using env variables to configure Mobilizon resource providers) I'm probably biased, but if we're just thinking about the configuration part, apart from having to specify Elixir types, the definition and configuration of Mobilizon's options look like an idiomatic RFC42-style NixOS module. |
I see what you mean, Mobilizon looks like a very complex app. |
Thanks for stepping in @RaitoBezarius! I don't see you in the
I think the most important thing right now is reviewing #119049 and see if it looks like something we're fine having as a settings format. |
nixos/tests/mobilizon.nix
Outdated
@@ -0,0 +1,24 @@ | |||
import ./make-test-python.nix ({ lib, ...}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely not saying you should do it here, but maybe add a comment on running the tests from the repo at some point?
I know it would require to repackage the app, so no need to do it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on what you mean by "from the repo"? Do you mean not using Elixir releases, or not using the NixOS module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being vague here.
When I meant tests from the repo, I was thinking about the test suite, that is run in elixir with mix test
. I realize though that I don't think you can do it with a release. You can probably abuse the mixRelease, and override to not output a release but just run mixTest.
Let me know if anything doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see! This seems like something we definitely want to do. I think it would be best to do it directly in the package derivation, or even better, to do it in mixRelease
. I don't know yet if Mobilizon wants a particular test environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only problem I see with doing it in the mixRelease, is that you need to compile your stuff twice, once for the tests and once for the final release.
}; | ||
|
||
http.port = mkOption { | ||
type = elixirTypes.port; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that correct? Did you add a port type in the elixirTypes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh is that a standard type that has been wrapped in an elixir type. Ok I think I get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is basically to allow the end user to put Elixir code if they want. That way, they can do:
{ pkgs, ... }:
let
format = pkgs.formats.elixirConf { };
in {
config.services.mobilizon.settings.":mobilizon"."Mobilizon.Web.Endpoint".http.port =
format.lib.mkGetEnv { envVariable = "MOBILIZON_LISTEN_PORT"; };
}
@minijackson you still seem to have an ofborg eval failure, https://gist.github.com/GrahamcOfBorg/fc3ab6595c4bd3155a2ecc221198ba39 |
About the runtime.exs issue, |
install -m 600 /dev/null "${secretEnvFile}" | ||
cat > "${secretEnvFile}" <<EOF | ||
# This file was automatically generated by mobilizon-setup-secrets.service | ||
export MOBILIZON_AUTH_SECRET='$(${evalElixir} "${genSecret}")' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just thinking about this. The problem here is that all the secrets go in the nix store right?
I'm not sure about the file itself, but this makes all the variables world readable.
The elixir commands used to generate the secrets could potentially come from another cli tool. As far as I know there is no strong requirements that the secrets get generated by elixir. Perhaps we should advise the user to generate his secret file himself and just accept it as a configuration input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that there is currently no way of specifying secrets except by asking the user to place a file somewhere outside of the nix store. This makes automated deployment more complicated, and I wanted a module where you could just enable the service and specify the configuration required by upstream, and it just works. There is no documentation in the Mobilizon documentation on how to generate the cookie, or how to generate the authentication secret, the instance secret, etc. This would be just extra steps needed to circumvent NixOS' particularities. At the end of the day, I thought the user didn't need to care about that, and that it could be automated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a worthwhile compromise, maybe just add a comment to clarify the approach?
one question, have you tried running the release with a systemd service? I'm asking because I'm running into a weird situation where I can start the release with the command line. But for some reason the systemd service keeps on craching with a weird error. It seems to be a problem with the file system being read only, but I can't figure out which path it's trying to write. This is what I have in the logs
I've verified that RELEASE_TMP is set to If you know a way to inspect the sys calls, or if you have some insights in debugging a similar issue, I'm interested. |
@happysalada Now that the blocking change to Only question left might be what you think about my change to This is of course bad news for Nix users, as every change to the source code is going to invalidate every single Mix dependency. Therefore I made the Would you like to have a separate PR for that or would you consider this uncontroversial enough to merge right away? If yes, just give me the sign to squash everything :) |
@@ -45,6 +47,14 @@ let | |||
runHook preConfigure | |||
|
|||
${./mix-configure-hook.sh} | |||
${if isNull appConfigPath then "" else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a nit, let's use optionalString here if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
I just have one nit (the one with optional string). regarding the change to build-mix, the honest answer is I have no idea. This looks good on principle so I would say, let's go ahead and merge it, and we will probably understand things better in the future. |
This is added for Mobilizon to be able to let it's dependencies read it's config directory
@happysalada Squashed and pushed :) |
afd2449
to
b7d448e
Compare
ofborg is failing with a weird error https://gist.github.com/GrahamcOfBorg/3f7d64ede074b68e0d68783babe0c3d9 |
Let's try reintroducing |
it doesn't look like this is related, it looks like ofborg is still failing. |
Another try: The eval error complains about not being able to read the |
Yay, it worked (with a sha256 for the offlineCache). I'll squash again. |
Co-Authored-By: Minijackson <[email protected]> Co-Authored-By: summersamara <[email protected]>
c45252e
to
799e90c
Compare
@erictapen thank you for sticking with this for so long! |
I can't believe this happened 😁 |
@erictapen thank you for actually making it to the end! This wouldn't have been possible without your continuous efforts. Sorry that I haven't been active in this PR lately… On a side note, can I delete the branch? |
Yes as far as im concerned |
FYI building the docs with mobilizon enabled yields those warnings:
|
Motivation for this change
Mobilizon is an online tool developed by Framasoft to help manage your events, your profiles and your groups.
This PR is based on #119049, and is a draft PR until #119049 is merged. I can't really make a PR to nixpkgs that merges into my own branch, so you will find #119049 commits in here too while it is open.
Note that the package and the module has a single maintainer, instead of the whole BEAM team, since I thought this was not a critical part of the BEAM ecosystem in Nixpkgs, and we might not want to have the BEAM team manage every BEAM packages and modules, but this is obviously up for discussion.
CC @NixOS/beam for this exact question, and since it is the first NixOS module integrating an Elixir service.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)