Remove all the deprecated setting fallback logic because it reached the target version and put a warning there if old configuration are still present #33707

lunny · 2025-02-24T19:54:35Z

The deprecated setting fallback logic should be removed once the target version is reached. A warning will be displayed if the old configuration items are still present.

…xpected version and put a warning there

delvh · 2025-02-24T20:33:45Z

modules/setting/config_provider.go

+func deprecatedSettingWarning(rootCfg ConfigProvider, oldSection, oldKey, newSection, newKey, version string) {
+	if rootCfg.Section(oldSection).HasKey(oldKey) {
+		LogStartupProblem(1, log.ERROR, "Deprecation: config option `[%s].%s` presents, please use `[%s].%s` instead because this fallback has been removed in %s", oldSection, oldKey, newSection, newKey, version)


Suggested change

func deprecatedSettingWarning(rootCfg ConfigProvider, oldSection, oldKey, newSection, newKey, version string) {

if rootCfg.Section(oldSection).HasKey(oldKey) {

LogStartupProblem(1, log.ERROR, "Deprecation: config option `[%s].%s` presents, please use `[%s].%s` instead because this fallback has been removed in %s", oldSection, oldKey, newSection, newKey, version)

// deprecatedSettingWarning is a warning about a setting that has already been removed, giving the user a last chance to fix their app.ini

func deprecatedSettingWarning(rootCfg ConfigProvider, oldSection, oldKey, newSection, newKey, version string) {

if rootCfg.Section(oldSection).HasKey(oldKey) {

LogStartupProblem(1, log.ERROR, "Deprecation: config option `[%s].%s` is still present, please use `[%s].%s` instead. This fallback has been removed in %s", oldSection, oldKey, newSection, newKey, version)

modules/setting/config_provider.go

delvh · 2025-02-24T21:45:07Z

modules/setting/config_provider.go

+// make linter happy when there is no deprecated setting at the moment
+var _ = deprecatedSetting
+
+func deprecatedSettingWarning(rootCfg ConfigProvider, oldSection, oldKey, newSection, newKey, version string) {


removedSetting?

Co-authored-by: delvh <[email protected]>

wxiaoguang

I think I have told you not only one time: such change breaks existing users.

Especially for some settings like LFS_CONTENT_PATH: the OLD DATA IS LOST!!! because you won't read the old config anymore.

lunny · 2025-02-25T04:36:46Z

I think I have told you not only one time: such change breaks existing users.

Especially for some settings like LFS_CONTENT_PATH: the OLD DATA IS LOST!!! because you won't read the old config anymore.

Do you have any idea how to improve it? Fatal if there is an old configuration? Or leave the fallback code forever?

wxiaoguang · 2025-02-25T04:44:05Z

Do you have any idea how to improve it? Fatal if there is an old configuration? Or leave the fallback code forever?

Some approaches in my mind:

Make Gitea fully manage the config file and migrate the config content.
- It might not work well with helmchart (just a guess).
Collect all deprecated config options and only "Fatal" once to output them together.
- There should be only one "Fatal" even if there are many deprecated config options.
- Otherwise if there are quite a few deprecated options, site admin would get stuck in "fatal -> modify config -> fatal -> modify config -> fatal" loop.

wxiaoguang · 2025-02-25T04:46:12Z

modules/setting/mailer_test.go

+			if strings.Contains(host, ":") {
+				addr, port, err := net.SplitHostPort(host)
+				assert.NoError(t, err)
+				sec.NewKey("SMTP_ADDR", addr)
+				sec.NewKey("SMTP_PORT", port)
+			} else {
+				sec.NewKey("SMTP_ADDR", host)
+			}


Why the tests are changed? Does it break?

The test was test the legacy configurations. Now the fallback code was removed. So the test was changed to test the new configurations.

Then I guess the test cases should be updated, but not change the config code here, unless you could clarify the config test code must be changed.

According to your change, the "HOST" key is gone, why you use the trick here to test the non-existing "HOST" config option?

lunny · 2025-03-01T20:34:50Z

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. Close this PR and leave an new issue for the future action.

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 24, 2025

lunny added this to the 1.24.0 milestone Feb 24, 2025

pull-request-size bot added the size/L label Feb 24, 2025

github-actions bot added the modifies/go Pull requests that update Go code label Feb 24, 2025

Remove all the deprecated setting fall backs because it reached the e…

564c670

…xpected version and put a warning there

lunny force-pushed the lunny/remove_fallback_keep_warning branch from 844adda to 564c670 Compare February 24, 2025 19:55

lunny changed the title ~~Remove all the deprecated setting fall backs because it reached the expected version and put a warning there~~ Remove all the deprecated setting fallback logic because it reached the target version and put a warning there if old configuration are still present Feb 24, 2025

delvh reviewed Feb 24, 2025

View reviewed changes

lunny and others added 4 commits February 24, 2025 14:12

Rename the function

5ab0de8

Merge branch 'main' into lunny/remove_fallback_keep_warning

1f62ea3

Apply suggestions from code review

ae166fd

Co-authored-by: delvh <[email protected]>

Add comment on removeSettingWarning

302bb33

delvh approved these changes Feb 24, 2025

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 24, 2025

Fix test

7e7811b

wxiaoguang requested changes Feb 25, 2025

View reviewed changes

GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 25, 2025

wxiaoguang reviewed Feb 25, 2025

View reviewed changes

lunny closed this Mar 1, 2025

lunny mentioned this pull request Mar 1, 2025

How to handle deprecated/removed config options #33761

Open

GiteaBot removed this from the 1.24.0 milestone Mar 1, 2025

go-gitea locked as resolved and limited conversation to collaborators May 31, 2025

Uh oh!

Remove all the deprecated setting fallback logic because it reached the target version and put a warning there if old configuration are still present #33707

Remove all the deprecated setting fallback logic because it reached the target version and put a warning there if old configuration are still present #33707

Uh oh!

Conversation

lunny commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wxiaoguang left a comment

Choose a reason for hiding this comment

Uh oh!

lunny commented Feb 25, 2025

Uh oh!

wxiaoguang commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunny commented Mar 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lunny commented Feb 24, 2025 •

edited

Loading