Support for setting a custom database name. Closes #936 #988
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
binaek
force-pushed
the
service_db_name_936
branch
from
0ec5c6f
to
9c6fc41
Compare
kaidaguerre
requested changes
Oct 6, 2021
db/db_local/install.go
Outdated
| } | ||
|
|
||
| func installSteampipeDatabaseAndUser(steampipePassword string, rootPassword string) error { | ||
| func installDatabaseAndSetupPermissions() (string, error) { |
There was a problem hiding this comment.
comment explaining what the return values are
There was a problem hiding this comment.
In fact I'd be tempted to pull out the database name logic and pass the name in from the calling code
functions should do 1 thing where possible
db/db_local/install.go
Outdated
| } | ||
|
|
||
| func installForeignServer() error { | ||
| func installForeignServer(dbName string) error { |
db/db_local/install.go
Outdated
| } | ||
| return sig | ||
| } | ||
| // func getInstalledBinarySignature() string { |
db/db_local/start_database.go
Outdated
| @@ -142,7 +142,7 @@ func StartDB(port int, listen StartListenType, invoker constants.Invoker) (start | |||
|
|
|||
| // startPostgresProcessAndSetPassword starts the postgres process and writes out the state file | |||
| // after it is convinced that the process is started and is accepting connections | |||
| func startPostgresProcessAndSetPassword(port int, listen StartListenType, invoker constants.Invoker) (e error) { | |||
| func startPostgresProcessAndSetPassword(port int, listen StartListenType, invoker constants.Invoker) (datName string, e error) { | |||
There was a problem hiding this comment.
why do we need named parameters?
db/db_local/start_database.go
Outdated
| } | ||
|
|
||
| err = postgresCmd.Process.Release() | ||
| if err != nil { | ||
| postgresCmd.Process.Kill() | ||
| return err | ||
| return "", err |
There was a problem hiding this comment.
what is this doing? add comment
postgresCmd.Process.Release()
db/db_local/start_database.go
Outdated
| } | ||
|
|
||
| // connect to the service and retrieve the database name | ||
| datName, err = retrieveDatabaseNameFromService(invoker) |
There was a problem hiding this comment.
and please use this consistently
db/db_local/start_database.go
Outdated
| @@ -142,7 +142,7 @@ func StartDB(port int, listen StartListenType, invoker constants.Invoker) (start | |||
|
|
|||
| // startPostgresProcessAndSetPassword starts the postgres process and writes out the state file | |||
| // after it is convinced that the process is started and is accepting connections | |||
| func startPostgresProcessAndSetPassword(port int, listen StartListenType, invoker constants.Invoker) (e error) { | |||
| func startPostgresProcessAndSetPassword(port int, listen StartListenType, invoker constants.Invoker) (datName string, e error) { | |||
| utils.LogTime("startPostgresProcess start") | |||
| defer utils.LogTime("startPostgresProcess end") | |||
There was a problem hiding this comment.
I think this function should read as follows: (with error handling of course)
func startPostgresAndSetPassword(port int, listen StartListenType, invoker constants.Invoker) (string, error) {
process, err := startPostgreProcess(port, listen, invoker)
databaseName, err = retrieveDatabaseName()
err := setPassword()
}
}
constants/env.go
Outdated
| @@ -5,6 +5,7 @@ const ( | |||
| EnvUpdateCheck = "STEAMPIPE_UPDATE_CHECK" | |||
| EnvInstallDir = "STEAMPIPE_INSTALL_DIR" | |||
| EnvConnectionString = "STEAMPIPE_CONNECTION_STRING" | |||
| EnvInstallDatabase = "STEAMPIPE_INSTALL_DATABASE" | |||
There was a problem hiding this comment.
why do we have STEAMPIPE_INSTALL_DATABASE and STEAMPIPE_DATABASE?
There was a problem hiding this comment.
STEAMPIPE_DATABASE is used by the remote connection layer to connect to Steampipe Cloud, using the API_KEY
kaidaguerre
requested changes
Oct 8, 2021
| _, err := executeSqlAsRoot(statements...) | ||
|
|
||
| for _, statement := range statements { | ||
| // NOTE: This may print a password to the log file, but it doesn't matter |
There was a problem hiding this comment.
I'm not sure I agree that it doesn't matter - can we do anything about this?
db/db_local/start_database.go
Outdated
| // after it is convinced that the process is started and is accepting connections | ||
| func startPostgresProcessAndSetPassword(port int, listen StartListenType, invoker constants.Invoker) (e error) { | ||
| func startPostgresProcessAndSetup(port int, listen StartListenType, invoker constants.Invoker) (databaseName string, e error) { |
db/db_local/create_client.go
Outdated
|
|
||
| return psqlInfo, nil | ||
| } | ||
|
|
||
| // createRootDbClient connects as a superuser to the | ||
| // installed database, if available, otherwise to the default | ||
| // "postgres" database | ||
| func createRootDbClient() (*sql.DB, error) { | ||
| utils.LogTime("db.createSteampipeRootDbClient start") | ||
| defer utils.LogTime("db™.createSteampipeRootDbClient end") | ||
|
|
There was a problem hiding this comment.
were you going to factor this out? I hate passing the empty name to createLocalDbClient
db/db_local/start_database.go
Outdated
| } | ||
| }() | ||
| // release the process - let the OS adopt it, so that we can exit | ||
| // DO AT END? IN DEFER? |
There was a problem hiding this comment.
remove/resolve this comment
// DO AT END? IN DEFER?
db/db_local/start_database.go
Outdated
| return ServiceStarted, err | ||
| } | ||
|
|
||
| // startPostgresProcessAndSetup starts the postgres process and writes out the state file |
db/db_local/start_database.go
Outdated
|
|
||
| err = setupServicePassword(invoker, password) | ||
| // set the password on the database | ||
| // we can't do this during installation, since the 'steampipe` user isn't setup yet |
There was a problem hiding this comment.
inconsistent quote marks - do not use slanty quotes in code comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we run steampipe for the first time, if it finds the environment variable
STEAMPIPE_INSTALL_DATABASEset to a non-empty value, it will install the database with the given name - if it's not present, or is empty, it will default tosteampipe