Skip to content

Conversation

@bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Jun 9, 2017

No description provided.

@bshaffer bshaffer requested a review from ryanmats June 9, 2017 22:12
@bshaffer bshaffer changed the title initial commit for CloudSQL postgres Add samples for CloudSQL postgres Jun 15, 2017
Copy link
Contributor Author

@bshaffer bshaffer left a 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');
Copy link
Contributor Author

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.

Copy link
Contributor

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"
Copy link
Contributor Author

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"

Copy link
Contributor

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) {
Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Contributor Author

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.

Copy link
Contributor

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
Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@bshaffer
Copy link
Contributor Author

Also looks like the build is failing because of trailing whitespace!

self::$gcloudWrapper->setDir($tmpDir);
chdir($tmpDir);

$connectionName = getenv('CLOUDSQL_CONNECTION_NAME');
Copy link
Contributor Author

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!

@bshaffer bshaffer merged commit 519cb6c into master Jun 20, 2017
@bshaffer bshaffer deleted the add-postgres-to-flex-cloudsql branch June 21, 2017 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants