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

do not use deprecatedSettingFatal for cosmetic reasons #26007

Closed
wants to merge 1 commit into from

Conversation

earl-warren
Copy link
Contributor

It breaks existing instances that would otherwise work perfectly fine. Failing to start an instance should only happen when there is a compelling reason to do so, for instance if the app.ini could not be modified in a way that is backward compatible. If the only motivation is to remove the setting for cosmetic reason, it must not be fatal.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 20, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 20, 2023
@lunny
Copy link
Member

lunny commented Jul 20, 2023

I think this is the original code in recent 4 or 5 versions. We have enough time to prompt administrator about the deprecated configurations.

If it doesn't break the instance, users will never check the message and never remove the deprecated configurations.

@wxiaoguang
Copy link
Contributor

Quote #23911 (comment)

Another problem is that I guess some "fatal" errors would make a lot of users reporting issues asking "why my Gitea doesn't start".
Maybe we need to use code to help users to upgrade their app.ini automatically.

I am neutral for this change. Either can be done:

  1. Revert the old behavior (no fatal)
  2. Automatically update the app.ini if it is writable
  3. Better "self-check" for various errors/warnings (Show deprecated config settings in UI #25994 (comment))

@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 Jul 20, 2023
modules/setting/lfs.go Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 20, 2023
modules/setting/lfs.go Outdated Show resolved Hide resolved
modules/setting/lfs_test.go Outdated Show resolved Hide resolved
It breaks existing instances that would otherwise work perfectly
fine. Failing to start an instance should only happen when there is a
compelling reason to do so, for instance if the `app.ini` could not be
modified in a way that is backward compatible. If the only motivation
is to remove the setting for cosmetic reason, it must not be fatal.

Refs: https://codeberg.org/forgejo/forgejo/issues/1081
@delvh
Copy link
Member

delvh commented Jul 20, 2023

Well… I am somewhat against removing this warning.
It is not just "cosmetic", it helps admins know what's going on in their instance.
I think it's better to make that explicit than that Gitea changes the config options slightly without notifying the admins who are left to wonder why something doesn't work anymore.
So it's not just "cosmetic", it serves a purpose.

I've started a survey among maintainers how we should handle config changes in the future.
We'll know the results of that in a little over a week (will post an issue describing how we will handle such changes then).
For 1.20, I think it's best to not change that warning, and simply use the new approach with 1.21.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 20, 2023
@lunny
Copy link
Member

lunny commented Jul 20, 2023

Maybe we can have another idea to prompt admin like have a top bar or others but not a fatal log. Or #25994

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Blocking per the ongoing maintainer discussion

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Jul 20, 2023
@wxiaoguang
Copy link
Contributor

@techknowlogick is there still disagreement about reverting it?

For 1.20.1 , quote: #26015 (comment)

I am neutral for it, but actually:

  • If it needs to wait, then wait for it
  • If it doesn't need to wait, then #26007 (this PR) doesn't seem useful after this release, because there might be a long time to next release.

@techknowlogick
Copy link
Member

@wxiaoguang Im wasn't sure what the conclusion of the chat was, so I figured waiting a moment before consensus was reached was ok.

@eeyrjmr
Copy link
Contributor

eeyrjmr commented Jul 21, 2023

I think this is the original code in recent 4 or 5 versions. We have enough time to prompt administrator about the deprecated configurations.

If it doesn't break the instance, users will never check the message and never remove the deprecated configurations.

yup, this "breaking" has been present for 1-2 years. It was fatal on windows when the [lfs] section was preferred but since it was mentioned in the release notes I didn't bother reporting it.

Carrying dead code is just technical debt and as long as it is clearly states what needs to be corrected there shouldn't be any problem as it is the duty of a site admin to review the changes and update accordingly. Likewise it is the duty of the gitea maintainers to ensure the release notes state this

#18358 This was 1st added (and broke on windows) in 1.17.0 July 31, 2022. 1 year ago, 3 releases ago

@wxiaoguang
Copy link
Contributor

Again: @techknowlogick is there still disagreement about reverting it?

What's the final conclusion?

@techknowlogick
Copy link
Member

@wxiaoguang I've had limited internet over the past days, but I think there is some decision by the maintainers that'll be acted upon.

lunny added a commit that referenced this pull request Jul 26, 2023
… file (#26094)

This PR includes #26007 's changes but have a UI to prompt administrator
about the deprecated settings as well as the log or console warning.
Then users will have enough time to notice the problem and don't have
surprise like before.

<img width="1293" alt="图片"
src="https://github.com/go-gitea/gitea/assets/81045/c33355f0-1ea7-4fb3-ad43-cd23cd15391d">

---------

Co-authored-by: wxiaoguang <[email protected]>
lunny added a commit that referenced this pull request Jul 26, 2023
… file (#26094) (#26154)

backport #26094 
Temporily resolve #25915
Related #25994

This PR includes #26007 's changes but have a UI to prompt administrator
about the deprecated settings as well as the log or console warning.
Then users will have enough time to notice the problem and don't have
surprise like before.

<img width="1293" alt="图片"
src="https://github.com/go-gitea/gitea/assets/81045/c33355f0-1ea7-4fb3-ad43-cd23cd15391d">
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants