Skip to content
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.

Sign up for GitHub

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

nixos/gitea: add WORK_PATH to config, fix 1.20 #241497

Merged
merged 1 commit into from
Aug 4, 2023
Merged

nixos/gitea: add WORK_PATH to config, fix 1.20 #241497

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions nixos/modules/services/misc/gitea.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ let
APP_NAME = ${cfg.appName}
RUN_USER = ${cfg.user}
RUN_MODE = prod
WORK_PATH = ${cfg.stateDir}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

GITEA_WORK_DIR = cfg.stateDir;

and

nixpkgs/nixos/modules/services/misc/gitea.nix

Line 665 in 687ed41

GITEA_WORK_DIR = cfg.stateDir;

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

services.gitea.settings = {
"cron.update_checker".ENABLED = lib.mkDefault false;

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^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bendlas bendlas Jul 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = value3

here, 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR but extraConfig is deprecated and we should remove it rather sooner than later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know.

Do our .ini config helpers do the unsectioned preamble?


${generators.toINI {} cfg.settings}

Expand Down