Web installer has some settings hardcoded

[0rzech]

• Gitea version (or commit ref): 1.2.3
• Git version: Not relevant
• Operating system: Not relevant
• Database (use [x]):
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
  • Not relevant
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (provide example URL)
  • No
  • Not relevant
• Log gist: Not relevant

Description

Gitea web installer has some settings hardcoded. As s seen in routers/install.go, some settings are not read from modules/setting/settings.go, eg:

• form.EnableOpenIDSignIn,
• form.EnableOpenIDSignUp,
• cfg.Section("log").Key("LEVEL").

This impedes automatic deployments.

Expected behavior

All installer settings should be read from setting module, which in turn should be a single source of truth for defaults and app.ini values.

[lunny]

Yes, that's why OpenID is disabled default but in fact not.

[strk]

It is intentional for OpenID to be disable by default for existing installations (for upgrades to not change behavior) and enabled by default for new installs (for new installs to have it on)

[lunny]

@strk I think disable them default should be better to keep it synchrony with default setting.

[0rzech]

@lunny @strk Gitea installer should use what users puts in their app.ini before installation. If one prepares app.ini with disabled openid and logs set to Warn, then this is what Gitea installer should pick. As of now, some settings are used properly and other are not because of hardcodes in installer. IMHO, there is no good reason to force some defaults on users who prepared their custom app.ini, is it?

[lunny]

@orzech yes, that's what I said. That should be a bug and maybe someone could send a PR to fix that. And I wish it could be back port to v1.3

[0rzech]

@lunny Sorry, I misread your comments. If you're fine with a novice in Go and in Web taking this task, then I can do it. I can't guarantee it will make on time for v1.3, though.

[strk]

Piotr if you do have an app.ini then the installer should not even run, or that's my understanding (I could be wrong).

[0rzech]

@strk The web installer will not show up when there is INSTALL_LOCK = true in app.ini. The file itself definitely can exist and be customized to user's preference.

[strk]

On Mon, Nov 27, 2017 at 12:00:33PM +0000, Piotr Orzechowski wrote: @strk The installer will not run when there is `INSTALL_LOCK = true` in `app.ini`. The `app.ini` can definitely exist and be customized to user's preference.

Can we then make app.ini also have OpenID on by default ? The only reason for it to be off is for when NO such setting is found in app.ini, which is the case for people who are upgrading from an older (pre-openid) version.

[lafriks]

And that would be not nice to them to enable it

[strk]

So anyway, in current master branch OPENID is enabled in custom/conf/app.ini.sample and there's no other app.ini other than the one for docker, so what discrepancy are we talking about ?

[0rzech]

The issue is with hardcoded values of some settings in web installer, not with having OpenID enabled or not.

[strk]

Sorry, you're right, those hard-coded values should not be there. Rather, new installs should have a way to tell IFF an app.ini file exists or not, or maybe install a default app.ini upfront (with no LOCK in it). That'd be the only way to have OpenID enabled by default even if absence of the directive would keep it disabled (for upgrades).

[strk]

Note there are other hard-coded values: "MySQL" as the default database and "git" as the default run user. PR #3010 fixes the OpenID ones (turning it on by default for anyone not opting out explicitly in config)

[0rzech]

Actually, MySQL is not hardcoded and neither seems 'git' user. Nevertheless there are other hardcoded settings indeed.

[strk]

"MySQL" is hardcoded on line 63 of routers/install.go it would surface if models.DbCfg.Type is set to "sqlite3" when models.EnableSQLite3 is false or to "tidb" when models.EnableTiDB is false. "git" is hardcoded on line 86 where for Windows runs it would be considered "the default user" (thus encoding a default locally). It seems to me that there are "install defaults" and "runtime defaults", and that both have a right to exist and can be disjoint.

[0rzech]

I recommend you run the setup process few times with changing various app.ini settings to see what's hardcoded and what's not. I am able to change database settings just by modifying app.ini before starting web installer. I think I was able to do that with run user too, but I don't remember now that well.

Install defaults may differ from runtime defaults, yes, but defaults != hardcodes. As of now, some settings can be changed successfully by modyfying app.ini before running web installer, while other values are silently ignored and hardcoded into it. And that should not happen.

App.ini should be loaded in whole, not in part - especially that some settings are not available in web installer at all. And even if they were, a user should not be forced to always click through some settings when custom app.ini is provided.

[strk]

Thank you for explanation, "silently ignored" is the key here, would be clearly a bug. Presence of a setting should be honoured. Please try my PR as of commit 6c8ff4a6a9d41600b5ff2f2fbdceb71b00489e58 and let me know how you like it.

[0rzech]

Thank you for explanation, "silently ignored" is the key here, would be clearly a bug.

You're welcome, but still the key here is "ignored at all". I wouldn't consider it fine behavior to "ignore loudly" either.

Presence of a setting should be honoured.

Yes, very much this.

Please try my PR as of commit 6c8ff4a and let me know how you like it.

Thanks. I'll post my results once done some simple tests.

[0rzech]

@lafriks This issue should not be closed yet, because there are still other hardcoded values in web installer.

[lafriks]

@0rzech this was closed automatically when I merged PR :)

[0rzech]

@lafriks Ah, sorry then. ;) Github should not do automated tasks under disguise of people. ;)

[0rzech]

@strk I've run web installer with your changes and my OpenID preferences were respected. Thanks.

[lunny]

@0rzech anything left on this issue?

[0rzech]

@lunny I think there is. There are whole groups of settings related to logging not covered by installer, for instance. As of now, the GUI supports file type only.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.