Allow ENABLE_OPENID_SIGNUP to depend on DISABLE_REGISTRATION #1369
Conversation
Omit the configuration variable (the default) to be dependent. Fixes go-gitea#1363
|
LGTM |
tboerger
added
the
lgtm/need 1
|
LGTM |
tboerger
added
lgtm/done
|
drone reporting broken ? build was successful... |
|
(comment fixed this - ready to merge) |
| @@ -203,7 +203,8 @@ IMPORT_LOCAL_PATHS = false | |||
| ; Whether to allow signin in via OpenID | |||
| ENABLE_OPENID_SIGNIN = true | |||
| ; Whether to allow registering via OpenID | |||
| ENABLE_OPENID_SIGNUP = true | |||
| ; Do not include to rely on DISABLE_REGISTRATION setting | |||
| ;ENABLE_OPENID_SIGNUP = true | |||
There was a problem hiding this comment.
Can we not default this to %(DISABLE_REGISTRATION) ?
There was a problem hiding this comment.
That would only work if it's renamed to DISABLE_OPENID_SIGNUP, which kinda makes sense anyhow.
There was a problem hiding this comment.
Uhm, I didn't know about %(DISABLE_REGISTRATION) - would that use the value of DISABLE_REGISTRATION found under custom dir too ?
Renaming a variable would be a compatibility break, then we'd need to migrate config files ?
There was a problem hiding this comment.
Migrating variablenames could be done by removing the old default from conf/app.ini but load it from custom/conf/app.ini if it exists. then cfg = oldVar; if exists(newVar) { cfg = newVar } in LoadConfig. Maybe throw a deprication warning. So no migration needed. And would make the config defautls easier. Since ENABLE_OPENID_SIGNUP = ! DISABLE_REGISTRATION which I don't think you can do in ini format 😒
There was a problem hiding this comment.
Are migration scripts allowed to modify stuff under custom/ ?
In general, is Gitea binary supposed to have write privilege in that dir ?
There was a problem hiding this comment.
@bkcsoft I just realized it doesn't take write access to custom/ to do what you're asking, which is basically still support the old variable in custom if present. Sounds a bit complex though, is that still worth the advantage being the possibility to write DISABLE_OPENID_REGISTRATION = %DISABLE_REGISTRATION in custom/conf/app.ini ?
lunny
added
the
type/feature
|
I think this is good enough to be merged. Has 2 LGTM and passed Drone. Fixes #1363 so please merge |
Omit the configuration variable (the default) to be dependent.
Fixes #1363