-
Notifications
You must be signed in to change notification settings - Fork 1k
Add samples for CloudSQL postgres #390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bshaffer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few changes needed.
| chdir($tmpDir); | ||
|
|
||
| $connectionName = getenv('CLOUDSQL_CONNECTION_NAME'); | ||
| $connectionName = getenv('MYSQL_CONNECTION_NAME'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should actually keep this one as CLOUDSQL_CONNECTION_NAME, since this is a string specific to cloudsql, and only relates to mysql in the form of a socket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| # set local mysql connection parameters | ||
| export MYSQL_DSN="mysql:host=127.0.0.1;port=3306;dbname=DATABASE" | ||
| export MYSQL_USERNAME=USER | ||
| export MYSQL_DSN="mysql:host=127.0.0.1;dbname=DATABASE;unix_socket=/cloudsql/CONNECTION_NAME" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original is correct, I got confused. So this should be:
export MYSQL_DSN="mysql:host=127.0.0.1;port=3306;dbname=DATABASE"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| // create the Silex application | ||
| $app = new Application(); | ||
|
|
||
| $app['pdo'] = function ($app) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be cleaner if we refactored this whole block to look like this:
// Create the PDO object for CloudSQL Postgres.
$user = getenv('POSTGRES_USER');
$password = getenv('POSTGRES_PASSWORD');
$dsn = getenv('POSTGRES_DSN');
$pdo = new PDO($user, $password, $dsn);
// Create the database if it doesn't exist
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$pdo->query('CREATE TABLE IF NOT EXISTS visits ' .
'(time_stamp TIMESTAMP DEFAULT CURRENT_TIMESTAMP, user_ip CHAR(64))');
// Add the PDO object to our Silex application.
$app['pdo'] = $pdo;WDYT? This way we could also remove the setting of the pgsql.* variables in index.php and the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done for Postgres + MySQL samples and re-tested everything
| use Symfony\Component\Yaml\Yaml; | ||
|
|
||
| class DeployTest extends \PHPUnit_Framework_TestCase | ||
| class DeployMySQLTest extends \PHPUnit_Framework_TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename these back to just DeployTest and LocalTest because we split them into separate directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| use Google\Cloud\TestUtils\FileUtil; | ||
| use Symfony\Component\Yaml\Yaml; | ||
|
|
||
| class DeployPostgresTest extends \PHPUnit_Framework_TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make these just DeployTest and LocalTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
Also looks like the build is failing because of trailing whitespace! |
| self::$gcloudWrapper->setDir($tmpDir); | ||
| chdir($tmpDir); | ||
|
|
||
| $connectionName = getenv('CLOUDSQL_CONNECTION_NAME'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually run the DeployTest on travis, these are run from Jenkins. But that being said, we still want to rename these. nice job!
No description provided.