How to handle deprecated/removed config options #14206

New Issue

GiteaMirror · 2025-11-02T11:06:18-06:00

GiteaMirror commented

2025-11-02 11:06:18 -06:00

Originally created by @lunny on GitHub (Mar 1, 2025).

Both a system failure and a critical issue can impact the admin user. To ensure awareness, we should implement a mandatory warning page that appears on every login, requiring the admin to acknowledge it before proceeding.

Originally posted by @lunny in https://github.com/go-gitea/gitea/issues/33707#issuecomment-2692406192

Originally created by @lunny on GitHub (Mar 1, 2025). Both a system failure and a critical issue can impact the admin user. To ensure awareness, we should implement a mandatory warning page that appears on every login, requiring the admin to acknowledge it before proceeding. _Originally posted by @lunny in https://github.com/go-gitea/gitea/issues/33707#issuecomment-2692406192_

GiteaMirror added the type/proposal label 2025-11-02 11:06:19 -06:00

GiteaMirror commented

2025-11-02 11:06:19 -06:00

@TheFox0x7 commented on GitHub (Mar 6, 2025):

After some work with huma they have a pretty nice system for body validation which I think might be reusable in this context. It doesn't quick fail when on bad arguments, but collects them instead into []error which gets returned to user. So my proposal for this is to do the following:

deprecatedSetting should return an error if the config option was removed more than n releases ago, otherwise it should just print a warning as it does now. If it's in error mode we can remove the workaround permanently.
Errors from each loader should be gathered and joined, if the resulting err!=nil, print it on fatal level and prevent running from startup
Set a clear cutoff period from where those warnings and errors are removed or move them to their own file (similar to migrations).

See: 5286afe149 for quick initial proposal (currently it lacks version comparison).
Only downside I think there is, is that in k8s and other autorestarting environments it will just endlessly reboot until someone will look at it - which is expected but a waste.

If we were to split removed settings into it's own file/func for clarity, it should be loaded before all the actual settings IMO providing early return.

unless this should come after some other refactor of settings? @wxiaoguang mentioned a rewrite of parsing package might be needed if I remember correctly?

@TheFox0x7 commented on GitHub (Mar 6, 2025): After some work with huma they have a pretty nice system for body validation which I think might be reusable in this context. It doesn't quick fail when on bad arguments, but collects them instead into `[]error` which gets returned to user. So my proposal for this is to do the following: 1. deprecatedSetting should return an error if the config option was removed more than `n` releases ago, otherwise it should just print a warning as it does now. If it's in error mode we can remove the workaround permanently. 2. Errors from each loader should be gathered and joined, if the resulting `err!=nil`, print it on fatal level and prevent running from startup 3. Set a clear cutoff period from where those warnings and errors are removed or move them to their own file (similar to migrations). See: https://github.com/TheFox0x7/gitea/commit/5286afe14957523856b9b1ff693d189247b3de2f for quick initial proposal (currently it lacks version comparison). Only downside I think there is, is that in k8s and other autorestarting environments it will just endlessly reboot until someone will look at it - which is expected but a waste. If we were to split removed settings into it's own file/func for clarity, it should be loaded before all the actual settings IMO providing early return. --- unless this should come after some other refactor of settings? @wxiaoguang mentioned a rewrite of parsing package might be needed if I remember correctly?

GiteaMirror commented

2025-11-02 11:06:20 -06:00

@wxiaoguang commented on GitHub (Mar 6, 2025):

I do not think it should stop starting when a "deprecated error" occurs. The deprecation could occur in many "load config" functions.

See case 2: https://github.com/go-gitea/gitea/pull/33707#issuecomment-2680478791

@wxiaoguang commented on GitHub (Mar 6, 2025): I do not think it should stop starting when a "deprecated error" occurs. The deprecation could occur in many "load config" functions. See case 2: https://github.com/go-gitea/gitea/pull/33707#issuecomment-2680478791

GiteaMirror commented

2025-11-02 11:06:20 -06:00

@TheFox0x7 commented on GitHub (Mar 6, 2025):

The way I understood it is that Fatal implies a critical error (here a config option is no longer used and was detected in the file) which in turn terminates server/aborts startup.
And to be exact I do not mean for setting which was deprecated a release ago to cause an error - 2nd release or later seems fair though.
This would avoid the loop you mentioned as the settings would load as normal but collect still configured ~~deprecated~~ removed settings and list them with Fatal once all settings are loaded.

@TheFox0x7 commented on GitHub (Mar 6, 2025): The way I understood it is that Fatal implies a critical error (here a config option is no longer used and was detected in the file) which in turn terminates server/aborts startup. And to be exact I do not mean for setting which was deprecated a release ago to cause an error - 2nd release or later seems fair though. This would avoid the loop you mentioned as the settings would load as normal but collect still configured ~~deprecated~~ removed settings and list them with `Fatal` once all settings are loaded.

GiteaMirror commented

2025-11-02 11:06:20 -06:00

@wxiaoguang commented on GitHub (Mar 6, 2025):

Sorry I misread, I thought "returning err" implies "fatal". Because there is another case, the setting package also has many technical debt:

Some "load" functions depends on others
Some "load" functions do changes (so if there is something wrong ahead, it shouldn't do the change anymore)

So ideally, when a non-removed-option error occurs, it should "fatal" as soon as possible to avoid other side-effects.

So to be honest, instead of "errors.Join", I think it's worth to use a separate error list to manage the "removed-option errors", and the "removed-option" check should be done before other "load" functions IMO.

@wxiaoguang commented on GitHub (Mar 6, 2025): Sorry I misread, I thought "returning err" implies "fatal". Because there is another case, the `setting` package also has many technical debt: * Some "load" functions depends on others * Some "load" functions do changes (so if there is something wrong ahead, it shouldn't do the change anymore) So ideally, when a non-removed-option error occurs, it should "fatal" as soon as possible to avoid other side-effects. So to be honest, instead of "errors.Join", I think it's worth to use a separate error list to manage the "removed-option errors", and the "removed-option" check should be done before other "load" functions IMO.

GiteaMirror commented

2025-11-02 11:06:20 -06:00

@TheFox0x7 commented on GitHub (Mar 7, 2025):

So something more like this then?
3750a2b8d0 (diff-f768bdf6f279e60b432e825f3ea54ebf81da85188c810e066128031393e5c749)

Though maybe it would be worth solving technical debt first so it doesn't pile up.

@TheFox0x7 commented on GitHub (Mar 7, 2025): So something more like this then? https://github.com/TheFox0x7/gitea/commit/3750a2b8d068456f76811ac1e86a0826367cdbf8#diff-f768bdf6f279e60b432e825f3ea54ebf81da85188c810e066128031393e5c749 Though maybe it would be worth solving technical debt first so it doesn't pile up.

GiteaMirror commented

2025-11-02 11:06:20 -06:00

@wxiaoguang commented on GitHub (Mar 8, 2025):

So something more like this then?
TheFox0x7@3750a2b#diff-f768bdf6f279e60b432e825f3ea54ebf81da85188c810e066128031393e5c749

Yup, it overall looks good.

@wxiaoguang commented on GitHub (Mar 8, 2025): > So something more like this then? > [TheFox0x7@3750a2b#diff-f768bdf6f279e60b432e825f3ea54ebf81da85188c810e066128031393e5c749](https://github.com/TheFox0x7/gitea/commit/3750a2b8d068456f76811ac1e86a0826367cdbf8#diff-f768bdf6f279e60b432e825f3ea54ebf81da85188c810e066128031393e5c749) Yup, it overall looks good.

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: github-starred/gitea#14206