-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
remove deprecated settings #33872
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
Conversation
ef4bb99 to
3cdf752
Compare
@wxiaoguang Please take a look at this approach
There was a problem hiding this 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:
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
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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.
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.
@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.
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.
Uh oh!
There was an error while loading. Please reload this page.
introduce a very basic removed settings management system
Related to: #33761
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.inifile 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