nixos/gitea: add WORK_PATH to config, fix 1.20 #241497
Conversation
this is in preparation for 1.20, which needs this option set
bendlas
force-pushed
the
prepare-gitea-120
branch
from
42aa718
to
47505ec
Compare
oof yes, done, thanks! |
| @@ -15,6 +15,7 @@ let | |||
| APP_NAME = ${cfg.appName} | |||
| RUN_USER = ${cfg.user} | |||
| RUN_MODE = prod | |||
| WORK_PATH = ${cfg.stateDir} | |||
There was a problem hiding this comment.
I was wondering if it wouldn't be worth pulling up the extraConfig line (${optionalString (cfg.extraConfig != null) cfg.extraConfig}) to before the main settings section.
That way, a non-sectioned setting like WORK_PATH could be added via extraConfig as well. Right now such a setting would get assigned randomly to whatever the last settings section turned out to be, right?
Do you see any trade-offs here, @emilylange?
There was a problem hiding this comment.
From go-gitea/gitea#25330
The "work path" priority is: WORK_PATH in app.ini > cmd arg --work-path > env var GITEA_WORK_DIR > builtin default
We seem to be doing something similar with GITEA_WORK_DIR at
nixpkgs/nixos/modules/services/misc/gitea.nix
Line 625 in 687ed41
and
nixpkgs/nixos/modules/services/misc/gitea.nix
Line 665 in 687ed41
Assuming the env var does not try to update the specified app.ini, this might be a more suitable place.
Or, alternatively, a lib.mkDefaulted WORK_PATH = cfg.stateDir in config similar to
nixpkgs/nixos/modules/services/misc/gitea.nix
Lines 391 to 392 in 687ed41
I don't know why one would want to manually override WORK_PATH when using the module, but ehh, maybe there are use-cases.
And both solutions would allow for that :)
I don't see much value changing the order of the freeform settings option and extraConfig.
But maybe I just don't quite get what you mean with that^^
There was a problem hiding this comment.
I don't really know what's going on either. If there is no WORK_PATH in app.ini, it will try to add it (possibly from that env var you're mentioning). That will fail when app.ini is read-only.
Maybe that could be considered a bug upstream, but I wanted to leave this here, so that we already know when doing the update.
There was a problem hiding this comment.
I don't see much value changing the order of the freeform settings option and extraConfig.
But maybe I just don't quite get what you mean with that^^
What I meant is: I couldn't add the WORK_PATH through extra config, because:
UNSECTIONED_CONFIG1 = value1
[section1]
SECTION_CONFIG = value2
EXTRA_CONFIG = value3here, EXTRA_CONFIG = value3 would be considered part of [section1], even though it's meant to be unsectioned. There is also no code that could reasonably rely on this behavior, since the order of sections is essentially random and so the section that an unsectioned extraConfig is assigned to would also be random.
Also I don't think there is a way to denote a "non-section" in order to add more unsectioned values, so in my books moving the extraConfig up would be strictly an improvement, because it would allow adding unsectioned values at all.
There was a problem hiding this comment.
TLDR but extraConfig is deprecated and we should remove it rather sooner than later.
There was a problem hiding this comment.
Good to know.
Do our .ini config helpers do the unsectioned preamble?
|
gitea 1.20 is not released yet, drafting again |
|
Not needed, see #243883 |
|
Huh, why was this closed? I ran into this issue today after updating and after finding this PR and locking the nixpkgs to the ref of #243883 (b61919e) the issue still happens so I think this is still needed, unless I'm missing something. Either that or remove the restriction of |
|
I assume you are using Forgejo via If so, the commit in question (b61919e) has been reverted in #244843 and should land in Also, on a related note: |
|
No, Im using just gitea and the issue i have is gitea wanting to set the missing WORK_PATH variable in its config, but failing because its read-only and aborting immediately. Which is also why im so confused this got closed in favour of #244843 because I dont see said fix there or a mention in the upstream changelog that the behaviour changed, i dont think the service type has anything to do with what this PR originally fixed? EDIT: as i was removing a deprecated entry from gitea.settings it still gives me that error but now stays active (so more of a warning i suppose) so im guessing this didnt cause the crash but using a deprecated setting ( |
|
Alright, sorry^^ |
I was assuming that since @Ma27 has been successfully running the updated version, that this change here was not needed after all and did not try myself yet #243883 (comment) I'll re-open until we figured out what's going on. @Ma27 can you comment: do you have |
bendlas
changed the title
|
🤦 Oof... I really feel like being struck my Murphy's law re tl;dr I get the same warning in my logs, so this PR is indeed needed. Please resolve the conflict in First of all, apologies! Let me explain why I missed this:
This is just an explanation, but I won't sugar-coat it, missing that issue was clearly my mistake! @maddiethecafebabe just to make sure I'm not misunderstanding you: currently your |
|
No worries! Yeah, the log message is marked as an error |
bendlas
force-pushed
the
prepare-gitea-120
branch
from
47505ec
to
5205c73
Compare
|
I remember it going into a restart loop, but that was also an earlier RC. Merge conflict resolved. |
Description of changes
Gitea and Forgejo 1.20 need a config option
WORK_PATHset and will try to update the config file, which will fail on NixOS where the config is read-only.Also add myself as maintainer to forgejo
TODO: Either test this with 1.19 or wait for 1.20 to be released before merging this.DONE: Tested together with 1.19.4-0 from #241777
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)