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.

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

remove deprecated settings #33872

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

TheFox0x7
Copy link
Contributor

@TheFox0x7 TheFox0x7 commented Mar 13, 2025

introduce a very basic removed settings management system

Related to: #33761


⚠️ BREAKING ⚠️

This removes configuration options that were deprecated more than 3 releases ago - up to Gitea 1.21.
See below for list of impacted options.

Impact on administrators of an instance

If user has any of the removed configuration options in their app.ini file at startup Gitea will log all issues and terminate. This will continue until all issues with configuration options are resolved.
As such the administrator of Gitea may experience downtime, constant restarting of the service or other startup issues depending on their configuration if they perform unattended upgrade to Gitea 1.24.

Otherwise there's no impact if there is no deprecated options in their configuration.

Keys or values preventing startup

Section Key value (if applicable)
service EMAIL_DOMAIN_WHITELIST -
mailer MAILER_TYPE -
mailer HOST -
mailer IS_TLS_ENABLED -
mailer DISABLE_HELO -
mailer SKIP_VERIFY -
mailer USE_CERTIFICATE -
mailer CERT_FILE -
mailer KEY_FILE -
mailer PROTOCOL smtp+startls
task QUEUE_TYPE -
task QUEUE_CONN_STR -
task QUEUE_LENGTH -
server ENABLE_LETSENCRYPT -
server LETSENCRYPT_ACCEPTTOS -
server LETSENCRYPT_DIRECTORY -
server LETSENCRYPT_EMAIL -
git.reflog ENABLED -
git.reflog EXPIRATION -
repository DISABLE_MIRRORS -
server LFS_CONTENT_PATH -
log XORM -
log ENABLE_XORM_LOG -
log ROUTER -
log DISABLE_ROUTER_LOG -
log ACCESS -
log ENABLE_ACCESSLOG -

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 13, 2025
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 13, 2025
@TheFox0x7 TheFox0x7 marked this pull request as ready for review March 14, 2025 22:34
@TheFox0x7
Copy link
Contributor Author

@wxiaoguang Please take a look at this approach

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

The last ask: are we ready and do we have enough courage to break many existing users' upgrade operation and force them to manually edit their config file? If yes, mark this PR as breaking and add a "breaking" section to tell users what would happen and what they should do.

Some "breaking" references:

@wxiaoguang
Copy link
Contributor

The last ask: are we ready and do we have enough courage to break many existing users' upgrade operation and force them to manually edit their config file? If yes, mark this PR as breaking and add a "breaking" section to tell users what would happen and what they should do.

also cc @go-gitea/maintainers

change tests to depend on length instead of error existance
@TheFox0x7
Copy link
Contributor Author

The last ask: are we ready and do we have enough courage to break many existing users' upgrade operation and force them to manually edit their config file?

I'd say yes. All those options where deprecated 3 or more releases ago and therefore could technically been removed anytime and unfortunately there's no premade solution for managing settings at this level.
This is better than endlessly supporting the legacy syntax and forces users to do a config cleanup.
Alternatively gitea could rewrite those options but frankly I'm not a fan of an app writing to it's configuration file - plus in k8s/ansible installs where the config is updated by user it would cause more confusion.

// Specifically default PATH to LFS_CONTENT_PATH
// DEPRECATED should not be removed because users maybe upgrade from lower version to the latest version
// if these are removed, the warning will not be shown
deprecatedSetting(rootCfg, "server", "LFS_CONTENT_PATH", "lfs", "PATH", "v1.19.0")
Copy link
Member

Choose a reason for hiding this comment

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

Can we still keep them at the original places? I think that will help code readers to find the history of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First version added errors to every setting loader function which would allow to keep the history, but as it has been explained - settings might do changes based on configuration options so early failure is better.

See #33761 (comment) for explanation and basis for this approach.

I do agree that it's less than ideal though.

Copy link
Member

Choose a reason for hiding this comment

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

Or we can put some comments in the original places as a reference?

@delvh
Copy link
Member

delvh commented Mar 15, 2025

The last ask: are we ready and do we have enough courage to break many existing users' upgrade operation and force them to manually edit their config file?

I'll stay neutral on this topic as I can see both advantages and disadvantages to this idea.
As stated previously, these config options have been deprecated for quite a long time (Gitea 1.19 was released on March 20, 2023, so two years ago).
At some point, we will have to remove the fallbacks.
The only question is: when do we remove them?
Now? In a year? In two years?
When will enough users have migrated to the new version that this operation will be fairly safe?
So far, I explicitly refrained from commenting on this PR as I expected you to ask exactly this question.

@wxiaoguang, how long would you keep them?
I do understand that 2 years is still comparably short, so I'm also fine with waiting another year or two with removing them.
But if someone doesn't keep their Gitea up to date in four years, they never will unless forced to.

@silverwind
Copy link
Member

These deprecations have been shown on the admin page for a long time, I'm fine with removing and calling out the alternatives in the release notes.

@silverwind silverwind added the type/deprecation Previously provided functionality is removed label Mar 16, 2025
@GiteaBot 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 Mar 16, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 16, 2025

@wxiaoguang, how long would you keep them? I do understand that 2 years is still comparably short, so I'm also fine with waiting another year or two with removing them.

Hmm yes, we had never introduced so many breaking changes in one release, TBH I don't know how end users would react to this. If they would feel frustrated for making so many changes in one release upgrade, they would give up and stay with the versions which have security vulnerabilities ......

So taking some progressive actions and waiting for more time seem reasonable.

@TheFox0x7
Copy link
Contributor Author

From my own pov - I did look through release notes (mainly for new features) and I haven't been maintaining app.ini on regular basis which after 2/3 releases was not a fun time (though it's because I did it "wrong" since I wanted to sync it with package manager example which was a terrible idea). So I'd argue that without a clear fault signal to the administrator no amount of warning will get everyone to migrate the configuration.

I'm fine with waiting for longer or for better ideas but this is a bridge that will need to be crossed eventually with a good future proof plan. Keeping deprecated config values forever is not feasible or a good idea in general.
We can postpone this for a rewrite of settings module.

I feel like 3 releases back is generally "good enough" - especially since it's technically major version change every time and there are cases where a flag/config is deprecated from 1.x -> 1.y and then removed in 2.0.0.


More related to the settings issue in general than PR:

I've also looked at existing systems (viper, koanf) but they won't provide what we need here (deprecation warnings and removal) so a custom solution would be required anyway.
Having a migration layer (similar to DB) - while it's a nice idea it cannot be applied on the config file directly, so it would require a cli endpoint to handle it (which is generally a good idea in the first place so solutions like ansible can use it to validate the configuration early).
I don't think a system where an old config (say 1.20 one) can be used for a newer version (1.24) and it automigrates itself in the background without user updating their configuration.

I'd like to help rewrite/refactor the settings system but for that we'd need to gather requirements - my own being TOML support and json schema for settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/deprecation Previously provided functionality is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants