-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 overwrite the log mode when installing (#25203) #25209
Do not overwrite the log mode when installing (#25203) #25209
Conversation
@@ -453,10 +453,9 @@ func SubmitInstall(ctx *context.Context) { | |||
|
|||
cfg.Section("session").Key("PROVIDER").SetValue("file") | |||
|
|||
cfg.Section("log").Key("MODE").SetValue("console") | |||
cfg.Section("log").Key("MODE").MustString("console") | |||
cfg.Section("log").Key("LEVEL").SetValue(setting.Log.Level.String()) | |||
cfg.Section("log").Key("ROOT_PATH").SetValue(form.LogRootPath) |
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.
By the way, shouldn't all these other keys be MustString
either?
It seems a bit strange that only the log isn't overwritten?`
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.
Nope.
setting.Log.Level
is from the config, so no overwritten
form.LogRootPath
is from user input (by default is also from the config)
- if user ever changed it, then it must overwrite
- if user didn't change it, still no overwritten
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.
Well, if we simply go by values that are explicitly set here, those are still [session].PROVIDER
, [repository.pull-request].DEFAULT_MERGE_STYLE
, and [repository.signing].DEFAULT_TRUST_MODEL
…
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.
Yes, there are still many legacy problems. I have searched these "SetValue" while I think we do not need to change too much at the moment.
Leave the problem to after the INI package refactoring.
And, end users could always prepare a full app.ini with "INSTALL_LOCK=true", then no overwriting problem anymore.
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.
How about to prevent user installing again from UI?
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.
- I have done so
- Improve install code to avoid low-level mistakes. Improve install code to avoid low-level mistakes. #17779
- Some users are doing fresh installation, the "app.ini" comes from docker environment variables, they do not "install again".
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.
I guess a backport is not the right place to discuss 😆
Backport #25203 by @wxiaoguang
Fix #24861