Refactor setup config #3605
Refactor setup config #3605
Conversation
|
✅ Hey nwmac! The commit authors and yourself have already signed the CLA. |
Codecov Report
@@ Coverage Diff @@
## v2-master #3605 +/- ##
==========================================
Coverage 51.42% 51.42%
==========================================
Files 725 725
Lines 20567 20567
Branches 3682 3682
==========================================
Hits 10577 10577
Misses 9990 9990 |
There was a problem hiding this comment.
Just a few minor comments. Have tested running the backend with...
- a fresh sqlite db and an existing old sqlite db
- a fresh mariadb db and an existing old mariadb db
- existing old postgres db cf service bound to a cf app (cf push with new code worked fine). Unfortunatly I couldn't directly access the db to confirm it's state but everything seems to work pre/post push of new code.
| showStratosConfig(consoleConfig) | ||
| portalProxy.Config.ConsoleConfig = consoleConfig | ||
| setSSOFromConfig(portalProxy, consoleConfig) | ||
| portalProxy.Config.SSOLogin = consoleConfig.UseSSO |
There was a problem hiding this comment.
The gate on SSO_LOGIN has been lost. Is this checked later on or not needed now?
There was a problem hiding this comment.
Can you expand what you mean by gate?
There was a problem hiding this comment.
Previously, in the removed setSSOFromConfig, Config.SSOLogin was only set if !portalProxy.Env().IsSet("SSO_LOGIN") was true. Now it's always set. Was wondering this was a fix or omission
There was a problem hiding this comment.
This is a fix - previously these two somewhat competed - now they are backed by the same env var
|
|
||
| var saveConsoleConfig = `INSERT INTO console_config (uaa_endpoint, console_admin_scope, console_client, console_client_secret, skip_ssl_validation, is_setup_complete, use_sso) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7)` | ||
| var deleteConsoleConfig = `DELETE FROM console_config` |
There was a problem hiding this comment.
I couldn't find where this is used. Should we be deleting the content on migration? For the next release should we nuke the table (assuming we only support point to point upgrades)?
|
|
||
| // Get the current value from the enfironment | ||
| if currentValue, ok := envLookup.Lookup(envVarName); ok { | ||
| shouldStore = currentValue != value |
There was a problem hiding this comment.
Think this also needs some kind of falsy == falsy check. When running the new code against an old sqlite console_config table it migrates CONSOLE_CLIENT_SECRET with an empty value. This is the only value in the new config table. Guessing this is due to a nil and an empty string or something?
Update: CONSOLE_CLIENT_SECRET didn't exist in either config.properties or default config.properties.
| } | ||
|
|
||
| if config == nil { | ||
| log.Info("Can not migrate setup data - setup table is empty") |
There was a problem hiding this comment.
Need to also run configStore.DeleteValue(systemGroupName, configSetupNeededMarker) here. Saw an issue where on a fresh db (mysql) the __CONFIG_MIGRATION_NEEDED remained and console_config migration ran on start up each time
nwmac
added
comments-addressed
ready for review
and removed
needs attention
|
Closed in favour of other PR |
No description provided.