Allow db user and password config to lampapp plugin #150
Conversation
| this.databaseName = options.databaseName || 'lampdb'; | ||
| this.databaseUser = options.databaseUser || 'root'; |
There was a problem hiding this comment.
See line 112 below. It was not actually using this.databaseUser (lampdb) but just 'root'. so changing default to root now so that default behavior does not change.
| @@ -38,7 +38,7 @@ class Drupal extends LAMPApp { | |||
| constructor(container, options) { | |||
| super(container, options); | |||
|
|
|||
| this.databaseName = constants.DRUPAL_DATABASE_NAME; | |||
There was a problem hiding this comment.
These constants seemed pointless to me. They just make it harder to see what the defaults actually are.
| 'DATABASE_PASS='+this.databasePass, | ||
| 'if [ -e $ASSET_DIR/credentials.sh ]; then source $ASSET_DIR/credentials.sh; fi', // Set db credentials if found. | ||
| 'mysql -e \'create database \'$DATABASE_NAME', | ||
| 'if [ "$DATABASE_USER" != "root" ]; then mysql -e \'create user "\'$DATABASE_USER\'"@"localhost" identified by "\'$DATABASE_PASS\'"\'; fi', |
There was a problem hiding this comment.
Kind of weird that we had the option for DatabaseUser but no command to actually create that user.
| @@ -96,7 +95,7 @@ class WordPressApp extends LAMPApp { | |||
| // number of the wp-settings include is because we want table prefix after | |||
| // all other settings but before that include. | |||
| this.script = this.script.concat([ | |||
| `if [ -a "/var/www/html/wp-config.php" ] ; then`, | |||
| `if [ -e "/var/www/html/wp-config.php" ] ; then`, | |||
There was a problem hiding this comment.
-a is deprecated in favor of -e
| @@ -105,7 +104,8 @@ class WordPressApp extends LAMPApp { | |||
|
|
|||
| // The boilerplate is significantly more simple. | |||
| this.script = this.script.concat([ | |||
| `if [ ! -a "/var/www/html/wp-config.php" ] ; then`, | |||
| 'mkdir -p /var/www/html', | |||
There was a problem hiding this comment.
At least for my testing, it was problematic to write to the wp-config.php if html dir didn't exist yet.
| * #!bin/bash | ||
| * DATABASE_NAME='foo' | ||
| * DATABASE_USER='bar' | ||
| * DATABASE_PASS='baz' |
There was a problem hiding this comment.
The password here doesn't seem that important since it's a temporary machine and the current password is 'strongpassword'. Could we drop the code that sets a password on a new mysql user and default to an empty string or 'strongpassword'?
Then we just need to document it.
There was a problem hiding this comment.
I put in line 17 comment that the default is strongpassword (we'll need to update docs as well) and then on line 48 make that the default, so I think we're ok.
I think the issue with allowing people the option to specify a password (as mentioned in #65) was really born from the fact that we never documented what the default db name, user and password were.
I guess the ability to customize the password is really not necessary but since we're letting them customize the db name and user (equally unnecessary) it makes sense to have all three.
lib/plugins/TaskRunner/LAMPApp.js
Outdated
| 'mysql -e \'flush privileges\'', | ||
| 'set -ux', |
There was a problem hiding this comment.
Based on how it's used, the database password isn't a secret and doesn't need to be blocked. Also, we typically block sensitive data at another point in the process because it's really helpful to see all the commands that occur in the build process.
We could also print the command with asterisks for the database password.
There was a problem hiding this comment.
I meant that I think we should remove the 'set +ux' above this and in one other place because it will prevent the commands from being written to the logs, right? We are currently writing this info to the logs.
I think It would be better for end user debugging to filter out secrets and replace them with asterisks instead of not showing the commands. We can do that later in the process which is what happens for some other data.
Also, the db name, user, and password will be repeated in the Drupal plugin (and probably the wordpress plugin) when writing the settings files.
There was a problem hiding this comment.
I think I covered this in d662c08
There was a problem hiding this comment.
Sorry, somehow I missed that change. Looks good.
No description provided.