Skip to content

settings refactoring #5070

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

Open
renothing opened this issue Oct 11, 2018 · 11 comments
Open

settings refactoring #5070

renothing opened this issue Oct 11, 2018 · 11 comments
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.

Comments

@renothing
Copy link
Contributor

there are so many global options for gitea. and the variables grew up more and more. it's so boring for upgrading, user have to check every config variable before upgrade. maybe it's better to make a distinction. we can make two part:

  • 1: readonly: like database settings, ssh server settings, domains
  • 2: writeable: like smtp, security etc...

and only keep part 1 in config file or read from enviroments, write part2 into database with default values.
then we can keep the docker image clearly and upgrading safe than before.

What do you think?

@techknowlogick techknowlogick added type/refactoring Existing code has been cleaned up. There should be no new functionality. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Oct 11, 2018
@lafriks
Copy link
Member

lafriks commented Oct 14, 2018

I would like to see writeable settings moved to configurable storage (file, databse, etc)

@lunny
Copy link
Member

lunny commented May 9, 2023

I think we already start moving part of configurations to database.

@TheFox0x7
Copy link
Contributor

I have something related to this proposal since the topic of refactoring settings is really broad. Can we consider taking a look at making better use of sections? Also using toml1 but that's another can of worms which should gets its own issue and discussion, if that hasn't happened yet.

To maybe show what I'm talking about take a look at ui settings and compare it to server. UI is more separated into sections making it smaller to look through, while server feels bloated and very large

Server would IMO benefit from making it more compact - for example all ssh related settings could easily be separated into server.ssh. Of course it's a two edge sword - too many sections make the overall readability worse.
Similar situation is with server.LFS_* parameters, shouldn't they be in [lfs], or at least [server.lfs]?
The other thing would be standardizing ENABLE_/DISABLE_ - to use only one of those. Personally I find ENABLE_CODE_PAGE = true more readable than than DISABLE_CODE_PAGE = false.

Is there a document/draft on the direction for settings? Maybe such ideas were discussed before and I missed it.

Footnotes

  1. main benefit would be possibility of having json-schema for settings and have taplo take advantage of it. Possibly even having a cue (or similar) layer to validate config or check for deprecated/invalid keys, which is related to https://github.com/go-gitea/gitea/issues/2762

@wxiaoguang
Copy link
Contributor

Actually as now, question is not "whether we should do it", but actually "how to do it clearly without breaking and it must succeed".

Take the "removal of jQuery" as an example, everyone agrees, it is much simpler than "refactoring settings" (no need to consider about breaking), it still takes very long time and it still doesn't get full success, various regressions during the refactoring. The real challenge is that some legacy code needs to be completely rewritten (simple search&replace doesn't work), it seems that only me is the person who keeps really rewriting legacy code .........


The same question as jQuery removal: if we start the "settings refactoring", we can't stop halfway, otherwise it would be a bigger disaster.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 22, 2024

And there are a lot of deprecated settings which "should be removed in 1.19/1.20/1.21/....", I had never seen who really started removing them

Please forgive me about my straightforward: are there enough developers having interests/time on this? It is not an easy task.

Screenshot: the deprecated settings

image

@lunny
Copy link
Member

lunny commented Dec 22, 2024

And there are a lot of deprecated settings which "should be removed in 1.19/1.20/1.21/....", I had never seen who really started removing them

Please forgive me about my straightforward: are there enough developers having interests/time on this? It is not an easy task.

Screenshot: the deprecated settings

image

I think I should add a task for releasing issue so that we can check them once we will release a new version.

@wxiaoguang
Copy link
Contributor

I think I should add a task for releasing issue so that we can check them once we will release a new version.

Have you thought it through about how to improve the config options without breaking existing users?

@lunny
Copy link
Member

lunny commented Dec 22, 2024

How about moving most of the configurations to the database? We can have a notification or top bar about settings break changes for everyone and only the administrator can visit the configuration admin page to change them.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 22, 2024

How about moving most of the configurations to the database? We can have a notification or top bar about settings break changes for everyone and only the administrator can visit the configuration admin page to change them.

But I do not see you have the determination, for example: #32288 (comment)

@TheFox0x7
Copy link
Contributor

It would need an /api/admin route, though I think it already is a thing to read config. There's a benefit of that with being able to make a validation layer on the request and making the config resistant to misconfiguration - such as using a storage which doesn't exist, etc.

On the other hand, it makes reading and updating the configuration more difficult as it essentially makes 2 sources for it - the config which still needs to exist for init and the database for other parts, so the end user who would want an 1:1 replica would find it more difficult. And it would require an request/cli command instead of looking though template and restarting.
I'm not the largest fan of moving settings into a database though there are benefits of that. Is there any larger application which does this?

Also based on your comments it seems that config in db already is a thing, but it's the first I hear of it. I might not be only one who had no idea that there's something besides app.ini for configuration. (Found it, system_setting table)


The real challenge is that some legacy code needs to be completely rewritten (simple search&replace doesn't work), it seems that only me is the person who keeps really rewriting legacy code .........

I'd be happy to help but I don't have a good grasp at what's legacy and what isn't. If you have some areas which might be worth looking at refactors/rewrites I can take a look at them.

Have you thought it through about how to improve the config options without breaking existing users?

With app.ini one option would be to have a "simple" script in python or something which would move keys to their new places. Not the cleanest/greatest idea but an idea.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 22, 2024

Some brief thoughts about the problems (not deeply, just to share):

  1. We need a new INI file reader/writer first (I think we need to write it by ourselves)
    • It should be able to handle all legacy INI package quirks
    • It should be able to fully manage the INI files (including keeping the comments)
    • We could strictly check the config file to find all mistakes.
  2. Fully manage the "app.ini" by the Gitea program
    • Migrate legacy options to new sections if possible, or remove them
    • Report fatal error if an option is not migrate-able and let site admin to handle it manually (this is really a bad user experience and we should avoid it as much as possible)
  3. Classify the config options in "app.ini"
    • Prerequisite: without them, Gitea won't start: various paths, DB config, queue config, storage, some secrets, etc
    • Internal configs: although users could set them, actually they are only used by Gitea internally and could be fully managed by Gitea itself: internal token, jwt secret, etc
    • Database config options: for example: most UI settings, mailer, etc. They could be fully stored in database.
    • Git hook related configs: I haven't really looked into it, but I guess we need to support it: some options could/should be saved in database, but the git hook sub-command needs it, maybe we need to sync the config from database config to ini config and/or find some other approaches.

I think step 1 is a must, it could help to avoid various regressions. Without it, I have no confidence to refactor the setting system

project-mirrors-bot-tu bot pushed a commit to project-mirrors/forgejo-as-gitea-fork that referenced this issue Jan 23, 2025
Translations update from [Codeberg Translate](https://translate.codeberg.org) for [Forgejo/forgejo](https://translate.codeberg.org/projects/forgejo/forgejo/).

Current translation status:

![Weblate translation status](https://translate.codeberg.org/widget/forgejo/forgejo/horizontal-auto.svg)

<!--start release-notes-assistant-->

## Draft release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Localization
  - [PR](https://codeberg.org/forgejo/forgejo/pulls/5070): <!--number 5070 --><!--line 0 --><!--description aTE4bjogdXBkYXRlIG9mIHRyYW5zbGF0aW9ucyBmcm9tIENvZGViZXJnIFRyYW5zbGF0ZQ==-->i18n: update of translations from Codeberg Translate<!--description-->
<!--end release-notes-assistant-->

Co-authored-by: earl-warren <[email protected]>
Co-authored-by: Xinayder <[email protected]>
Co-authored-by: Gusted <[email protected]>
Co-authored-by: Kita Ikuyo <[email protected]>
Co-authored-by: Fjuro <[email protected]>
Co-authored-by: 0ko <[email protected]>
Co-authored-by: hugoalh <[email protected]>
Co-authored-by: Outbreak2096 <[email protected]>
Co-authored-by: Eryk Michalak <[email protected]>
Co-authored-by: Caesar Schinas <[email protected]>
Co-authored-by: hankskyjames777 <[email protected]>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5070
Reviewed-by: Earl Warren <[email protected]>
Co-authored-by: Codeberg Translate <[email protected]>
Co-committed-by: Codeberg Translate <[email protected]>
(cherry picked from commit 45198ce)
(cherry picked from commit b73fd55)
project-mirrors-bot-tu bot pushed a commit to project-mirrors/forgejo-as-gitea-fork that referenced this issue Jan 23, 2025
Translations update from [Codeberg Translate](https://translate.codeberg.org) for [Forgejo/forgejo](https://translate.codeberg.org/projects/forgejo/forgejo/).

Current translation status:

![Weblate translation status](https://translate.codeberg.org/widget/forgejo/forgejo/horizontal-auto.svg)

<!--start release-notes-assistant-->

## Draft release notes
<!--URL:https://codeberg.org/forgejo/forgejo-->
- Localization
  - [PR](https://codeberg.org/forgejo/forgejo/pulls/5070): <!--number 5070 --><!--line 0 --><!--description aTE4bjogdXBkYXRlIG9mIHRyYW5zbGF0aW9ucyBmcm9tIENvZGViZXJnIFRyYW5zbGF0ZQ==-->i18n: update of translations from Codeberg Translate<!--description-->
<!--end release-notes-assistant-->

Co-authored-by: earl-warren <[email protected]>
Co-authored-by: Xinayder <[email protected]>
Co-authored-by: Gusted <[email protected]>
Co-authored-by: Kita Ikuyo <[email protected]>
Co-authored-by: Fjuro <[email protected]>
Co-authored-by: 0ko <[email protected]>
Co-authored-by: hugoalh <[email protected]>
Co-authored-by: Outbreak2096 <[email protected]>
Co-authored-by: Eryk Michalak <[email protected]>
Co-authored-by: Caesar Schinas <[email protected]>
Co-authored-by: hankskyjames777 <[email protected]>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5070
Reviewed-by: Earl Warren <[email protected]>
Co-authored-by: Codeberg Translate <[email protected]>
Co-committed-by: Codeberg Translate <[email protected]>
(cherry picked from commit 45198ce)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

No branches or pull requests

6 participants