nixos/{act,gitea-actions}-runner: refactor, nixos/forgejo-runner: init#485596
nixos/{act,gitea-actions}-runner: refactor, nixos/forgejo-runner: init#485596SigmaSquadron wants to merge 16 commits intoNixOS:masterfrom
Conversation
gepbird
left a comment
There was a problem hiding this comment.
Thanks!
Probably we should update some of the references:
- nixos/tests/forgejo.nix
- https://wiki.nixos.org/wiki/Forgejo#Actions_Runner
- https://forgejo.org/docs/next/admin/actions/runner-installation/
Even with whitespaces hidden, the diff is a little too big in my opinion. Some things that could be split up:
- using lib.getExe
- introducing
${name}and${prettyName} - the main change that introduces genAttrs
I'm not sure if this is the go-to implementation, this still lives in gitea-actions-runner.nix, but on the other hand duplicating all the code would be worse.
|
Yeah there might be better ways to implement this. Something else I tried first was to have something like this: {
options.services.forgejo-runner = magicFunctionThatRecursivelyChangesMentionsToGiteaToForgejoInDescriptionAttributes options.services.gitea-actions-runner;
}but that turned out to be pretty complicated. A change that can help with the diff is to move the existing config to |
What about using a generic gitea base module that takes an "implementation" argument that is forgejo or gitea (this would contain the part that you have currently in |
5c60724 to
32ab262
Compare
|
Tests are failing, and it's late. I'll pick this up tomorrow if I can, and implement the |
32ab262 to
52d5164
Compare
it's not that I lied about going to sleep, it's that I failed. Implemented the new generic.nix file, and it ended up being very elegant indeed. |
TODO:
After merge
|
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
52d5164 to
5169a18
Compare
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
…statement in config Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
This removes two functions called using `lib.*` instead of from the inherit header. Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
…ctions and deduplicate checks Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
…on binary path Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
…documentation Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
5169a18 to
bca1eec
Compare
|
Tests no longer failing. @ofborg test forgejo.sqlite3 forgejo.postgres forgejo.mysql forgejo-lts.sqlite3 forgejo-lts.postgres forgejo-lts.mysql |
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
There was a problem hiding this comment.
Thanks for splitting it up into small chunks, it was much easier to review.
Diff looks pretty good, just to be sure I'll soon check the docs and try this on my server.
After merge
* [ ] Wiki update * [ ] PR against Forgejo's docs with the new interface
We should backport this PR to not confuse stable users after the doc updates.
| { | ||
| assertion = anyWantsContainerRuntime -> hasDocker || hasPodman; | ||
| message = "Label configuration on gitea-actions-runner instance requires either docker or podman."; | ||
| message = "At least one of the configured Gitea Actions Runner instances require a container hypervisor, but neither Docker nor Podman are enabled."; |
There was a problem hiding this comment.
Grammar: require -> requires
| @@ -1,3 +1,11 @@ | |||
| let | |||
| attributeName = "gitea-actions-runner"; | |||
| mainProgram = "act_runner"; | |||
There was a problem hiding this comment.
Shouldn't this be retrieved from pkgs.${attributeName}.meta.mainProgram?
| let | ||
| mkRunnerService = | ||
| name: instance: | ||
| id: instance: |
There was a problem hiding this comment.
Nit: maybe serviceName or instanceName is a clearer? id is also good since it's short.
| attributeName = "forgejo-runner"; | ||
| name = "forgejo"; | ||
| prettyName = "Forgejo"; | ||
| runnerPrettyName = "Forgejo Runner"; | ||
| srcUrl = "https://code.forgejo.org/forgejo/runner"; | ||
| docsUrl = "https://forgejo.org/docs/latest/user/actions/overview"; | ||
| labelsUrl = "https://forgejo.org/docs/latest/admin/actions"; |
There was a problem hiding this comment.
Nit: preserve the same order of arguments as the gitea runner. This order seems more logical so I think the other should be changed.
| { | ||
| assertions = [ | ||
| { | ||
| assertion = any tokenXorTokenFile (attrValues cfg.instances); |
There was a problem hiding this comment.
Unrelated to your changes, but there's a bug here. This only throws, if all the configs are incorrect (in other words it only does NOT throw when there is at least one correct config), it should throw if it finds at least one incorrect config.
Example: this config builds when it should throw instead, because the existing "test" instance is correct, it ignored "test2" being incorrect with both token and tokenFile set:
diff --git a/nixos/tests/forgejo.nix b/nixos/tests/forgejo.nix
index 85221a33c9ee..98d9928bd39d 100644
--- a/nixos/tests/forgejo.nix
+++ b/nixos/tests/forgejo.nix
@@ -72,6 +72,17 @@ let
];
tokenFile = "/var/lib/forgejo/runner_token";
};
+ configuration.services.forgejo-runner.instances."test2" = {
+ enable = true;
+ name = "ci";
+ url = "http://localhost:3000";
+ labels = [
+ # type ":host" does not depend on docker/podman/lxc
+ "native:host"
+ ];
+ tokenFile = "/var/lib/forgejo/runner_token";
+ token = "fake-token";
+ };
};
specialisation.dump = {
inheritParentConfig = true;| serviceConfig = { | ||
| DynamicUser = true; | ||
| User = "${name}-runner"; | ||
| StateDirectory = "${name}-runner"; |
There was a problem hiding this comment.
When existing forgejo-runner users switch to the new module, the state directory will be changed. I'm not sure if the data loss is significant, I assume it can be recreated without issues and with automatic runner re-registrations.
This is my current data:
[root@raspi5-doboz:/var/lib/gitea-runner/raspi5-doboz]# l
total 20K
drwxr-xr-x 3 nobody nogroup 4.0K Dec 31 02:06 .
drwxr-xr-x 5 nobody nogroup 4.0K Dec 30 18:20 ..
drwxr-xr-x 4 nobody nogroup 4.0K Dec 31 01:28 .cache
-rw-r--r-- 1 nobody nogroup 64 Dec 31 02:06 .labels
-rw-r--r-- 1 nobody nogroup 478 Dec 31 02:06 .runner
[root@raspi5-doboz:/var/lib/gitea-runner/raspi5-doboz]# cat .labels
ubuntu-24.04-arm:docker://ghcr.io/catthehacker/ubuntu:act-24.04
[root@raspi5-doboz:/var/lib/gitea-runner/raspi5-doboz]# cat .runner
{
"WARNING": "This file is automatically generated by forgejo-runner. Do not edit it manually unless you know what you are doing. Removing this file will cause act runner to re-register as a new runner.",
"id": 8,
"uuid": "d2000000-0000-0000-0000-000000000000",
"name": "raspi5-doboz",
"token": "b200000000000000000000000000000000000000",
"address": "https://git.tchfoo.com",
"labels": [
"ubuntu-24.04-arm:docker://ghcr.io/catthehacker/ubuntu:act-24.04"
]
}There was a problem hiding this comment.
I think the most impactful change will be losing the cache directory, since those can be large and are also used extensively in many folks' CI setups. So it will both duplicate the directory and will make folks' CI pipelines take longer, at least temporarily.
Therefore I would not want this PR backported, along with the concerns that emily mentioned below.
emilylange
left a comment
There was a problem hiding this comment.
Not only does this PR so far bypass the existing module maintainer and parties involved, it also shows a complete lack of context, prior and ongoing discussions and work.
I am thankful I somehow found this PR before merging, as otherwise this would have likely lead to an immediate revert.
I get that there are many different understandings of ownership in nixpkgs. But at an absolute minimum, please don't just ping pong PRs between you and your buddy without at least review requesting the existing meta.maintainers.
Major parts of this actively conflict with ongoing work, I am sorry.
I am blocking this as quasi co-maintainer of nixos/gitea-actions-runner alongside @mweinelt and on behalf of the Forgejo team (@adamcstephens, @bendlas, @christoph-heiss, @NyCodeGHG, @pyrox0, @tebriel).
|
I apologize for not pinging the maintainers, I take for granted that CI does it for package. If I remembered that, I would've pinged @mweinelt for the module and the forgejo team for the tests.
I did some basic searches beforehand to confirm there are no duplicate work with keywords like "gitea actions", "forgejo actions", "gitea in:title" and "forgejo in:title" and didn't find related issues or PRs. Would you mind linking the ongoing work? For bigger PRs like this I think letting it sit for a week is reasonable to give other people time to react.
Can you add yourself to the maintainer list? I totally understand you blocking this, and I'm sorry for this oversight. |
This may be just a wording issue, but I take offense at the accusation that "we're ping-ponging PRs" to bypass the usual review and ownership process. This was not ready for merge in any capacity at the time you blocked it, and neither of us intended to somehow usurp your authority on Forgejo. Hexa would at least be asked for a review before this is merged. My intention was, as with most PRs in Nixpkgs, to fix something that someone would benefit from and hasn't already been done.
It's okay to have a plan for the future of this module and such, but please understand that I cannot peer into your mind nor go through the mostly inactive Forgejo room hunting for this plan. There are no PRs or issues related to this module that even mention a major refactor. If you'd like others to keep away from a module while the Forgejo team works on it, please open a tracking issue, and everyone will know how to contribute to your vision instead of opening disparate PRs like these. I understand why you're blocking this, which is why this is closed, but I feel like you're assuming bad faith where there is none. I certainly hope you wouldn't have said the same to a new contributor. |
|
I feel like most of this could have prevented if #485707 had happened earlier or if this PR here was marked as draft or communicated as such. Instead, what I saw were two committers who just implemented and merged #483216, on their way to sprint through this PR here, without pinging anyone, not even I am sorry if I read the room on this wrong, but I genuinely feared that this may be merged in less than 24h. I really don't get what would have been so hard to ask virtually anyone involved prior to investing hours of time into this. Be it hexa to ask whether he is even interested in something like this. The sole purpose of your PR was to initialize and alias Please don't just add a module for a package without pinging the corresponding package maintainers. |
|
Sure, that's fair enough. Would you like #483216 reverted if you feel it hasn't gone through a thorough enough review? |
Makes the gitea-actions-runner module into a generic act-runner module, allowing forgejo to be aliased to it.
Would highly recommend reviewing commit-by-commit.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.