Reconsider move of settings to database #83

fnetX commented 2023年11月16日 22:33:46 +01:00

Recently, there was a start to shift certain settings to being available only in the database like the avatar / gravatar related settings. I am wondering: How are users able to toggle this without manual setting, e.g. when mass-provisioning Forgejo instances?

I propose that we really reconsider this step and maybe take a better way than Gitea, because I do not see any benefits. If you want to hot-reload settings, you could also implement settings reload, or write back toggled values to the config file.

An alternative would be an option to toggle and initialize these settings differently, but I cannot find something in the docs.

Recently, there was a start to shift certain settings to being available only in the database like the avatar / gravatar related settings. I am wondering: How are users able to toggle this without manual setting, e.g. when mass-provisioning Forgejo instances? I propose that we really reconsider this step and maybe take a better way than Gitea, because I do not see any benefits. If you want to hot-reload settings, you could also implement settings reload, or write back toggled values to the config file. An alternative would be an option to toggle and initialize these settings differently, but I cannot find something in the docs.

KaKi87 commented 2023年11月17日 02:30:35 +01:00

You're saying that avatar settings currently aren't stored in database ?

If I recall correctly, I didn't loose these settings when moving my instance from a server to another.

Maybe these are file-based ?

Anyways, I agree that storing in database instead would be better.

You're saying that avatar settings currently aren't stored in database ? If I recall correctly, I didn't loose these settings when moving my instance from a server to another. Maybe these are file-based ? Anyways, I agree that storing in database instead would be better.

fnetX commented 2023年11月17日 12:15:30 +01:00

No, it IS stored in database, and I disagree to this approach. I am not moving instance but mass-deploying, and manually clicking through the admin panel for hundreds of Forgejo instances is not a good solution.

No, it IS stored in database, and I disagree to this approach. I am not moving instance but mass-deploying, and manually clicking through the admin panel for hundreds of Forgejo instances is not a good solution.

KaKi87 commented 2023年11月17日 12:16:47 +01:00

I'm sorry I don't understand your request then.

I'm sorry I don't understand your request then.

earl-warren commented 2023年11月17日 13:55:58 +01:00

Recently, there was a start to shift certain settings to being available only in the database like the avatar / gravatar related settings. I am wondering: How are users able to toggle this without manual setting, e.g. when mass-provisioning Forgejo instances?

I propose that we really reconsider this step and maybe take a better way than Gitea, because I do not see any benefits. If you want to hot-reload settings, you could also implement settings reload, or write back toggled values to the config file.

An alternative would be an option to toggle and initialize these settings differently, but I cannot find something in the docs.

FWIW there has been no progress in migrating more settings to the database and to this date gravatar remains the only one. I also don't recall recent discussions related to hot reloading settings. The settings initialization code is very involved and refactoring it to be reloadable would be a major undertaking.

> Recently, there was a start to shift certain settings to being available only in the database like the avatar / gravatar related settings. I am wondering: How are users able to toggle this without manual setting, e.g. when mass-provisioning Forgejo instances? > > I propose that we really reconsider this step and maybe take a better way than Gitea, because I do not see any benefits. If you want to hot-reload settings, you could also implement settings reload, or write back toggled values to the config file. > > An alternative would be an option to toggle and initialize these settings differently, but I cannot find something in the docs. FWIW there has been no progress in migrating more settings to the database and to this date gravatar remains the only one. I also don't recall recent discussions related to hot reloading settings. The settings initialization code is very involved and refactoring it to be reloadable would be a major undertaking.

fnetX commented 2023年12月05日 23:30:46 +01:00

Which strategy can we endorse from Forgejo's side? The current state is not acceptable IMHO. We should either add a way to toggle the database settings programmatically, or revert the changes to rely on config again.

Not being able to provision an instance with good privacy settings violates our goal of having a privacy-respecting software IMHO.

Which strategy can we endorse from Forgejo's side? The current state is not acceptable IMHO. We should either add a way to toggle the database settings programmatically, or revert the changes to rely on config again. Not being able to provision an instance with good privacy settings violates our goal of having a privacy-respecting software IMHO.

Gusted commented 2023年12月05日 23:59:01 +01:00

From a technical viewpoint I still cannot make sense of the move to database as it's simply broken (The initialization code isn't run again when a value changes, so it ends up not working if computed values from the initialization code is required after a value change which causes panics) and hard to make sense of the logic (as seen in forgejo/forgejo#926). I really don't mind reverting it to simple boring config style. I don't have any indication that Gitea is going to pursue this seriously to make it actually work.

From a technical viewpoint I still cannot make sense of the move to database as it's simply broken (The initialization code isn't run again when a value changes, so it ends up not working if computed values from the initialization code is required after a value change which causes panics) and hard to make sense of the logic (as seen in https://codeberg.org/forgejo/forgejo/pulls/926). I really don't mind reverting it to simple boring config style. I don't have any indication that Gitea is going to pursue this seriously to make it actually work.

delvh commented 2023年12月06日 22:12:36 +01:00

Yeah, I'm also torn on this functionality:
In theory, if everything was converted, it might work out with live reloading, etc.
The live configuration change would be really nice, especially for large instances like Codeberg you cannot just shut down to change the config (as long as it is also possible to auto-configure it via file or something else).
But as it is now, I'm against it: It simply introduces a second way of configuration which is completely overkill as it is inconsistent where what can be applied.

Yeah, I'm also torn on this functionality: In theory, if everything was converted, it might work out with live reloading, etc. The live configuration change would be really nice, especially for large instances like Codeberg you cannot just _shut down_ to change the config (as long as it is also possible to auto-configure it via file or something else). But as it is now, I'm against it: It simply introduces a second way of configuration which is completely overkill as it is inconsistent where what can be applied.

Gusted commented 2025年02月11日 20:14:59 +01:00

Alright time to revisit this.

I've spoken a fair bit on this since its introduction and haven't made a PR reverting all of this because of the potential argument "Just wait and see how its result will look", but since its introduction there have has only been one new value that was moved to the dynamic settings for which I will argue that this benefit of having this being changed on the fly is nihil.

I see some very good reasons to drop and deprecate moving settings to the database after no good success:

• You can no longer set certain values in the app.ini and thus if you have automated provisioning of Forgejo instances, such values become inaccessible or at least very hard to configure.
• I do not believe that storing frequently requested values like this should be done stored in the database, sure caching works but that's just full circle of storing values in memory which is already the case.
• The effort to also support this way of configuring values would make forgejo/discussions#295 harder to accomplish.
• Nobody is pursuing to actually make this happening and I have not heard a usecase of why this is useful to have, the only time this ever comes up is during debugging of a bug. There's way too much fundamental stuff not done yet to even consider it as an experimental feature and the current existing code has no good justification of why some values should be dynamic and some others are not.

I propose adding a deprecation notice to v11 and add app.ini options for those that are currently stored in the database, then drop the database settings code in v12 branch.

Alright time to revisit this. I've spoken a fair bit on this since its introduction and haven't made a PR reverting all of this because of the potential argument "Just wait and see how its result will look", but since its introduction there have has only been [one new value](https://github.com/go-gitea/gitea/pull/29320) that was moved to the dynamic settings for which I will argue that this benefit of having this being changed on the fly is nihil. I see some very good reasons to drop and deprecate moving settings to the database after no good success: - You can no longer set certain values in the app.ini and thus if you have automated provisioning of Forgejo instances, such values become inaccessible or at least very hard to configure. - I do not believe that storing frequently requested values like this should be done stored in the database, sure caching works but that's just full circle of storing values in memory which is already the case. - The effort to also support this way of configuring values would make forgejo/discussions#295 harder to accomplish. - Nobody is pursuing to actually make this happening and I have not heard a usecase of why this is useful to have, the only time this ever comes up is during debugging of a bug. There's way too much fundamental stuff not done yet to even consider it [as an experimental feature](https://codeberg.org/forgejo/discussions/issues/246) and the current existing code has no good justification of why some values should be dynamic and some others are not. I propose adding a deprecation notice to v11 and add app.ini options for those that are currently stored in the database, then drop the database settings code in v12 branch.