Add postgres samples #396

ace-n · 2017-05-20T07:17:13Z

Don't merge this (yet): some region tags have changed, and the docs have not been updated accordingly.

codecov-io · 2017-05-20T07:22:34Z

Codecov Report

Merging #396 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #396   +/-   ##
=======================================
  Coverage   83.88%   83.88%           
=======================================
  Files           4        4           
  Lines         422      422           
=======================================
  Hits          354      354           
  Misses         68       68

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37a3cb2...5880a0c. Read the comment docs.

jmdobry

I feel like this PR is missing an update to the app.yaml file?

Also, why not collapse the MYSQL_ and POSTGRES_ environment variables in variables that are more generic, like DB_USER, DB_PASSWORD, etc. so you don't have to document two sets of variables? The one variable that would control which database to use would be that SQL_CLIENT variable.

jmdobry · 2017-05-22T23:01:43Z

appengine/cloudsql/package.json

        ],
        "msg": "Last 10 visits:",
-        "args": ["server.js"]
-      },
-      "deploy": {


Why remove this configuration?

The line below it seems to be MySQL-specific environment variables - but if we use the same environment variables for both clients, then I'll add it back in + update it accordingly.

jmdobry · 2017-05-22T23:02:21Z

appengine/cloudsql/server.js

 const crypto = require('crypto');

 const app = express();
 app.enable('trust proxy');
 // [END setup]
+let knex;
+
+function connectMysql () {


I feel like this can be simplified, do we really need two different connection methods?

I was wondering about this as well. The only difference between these two is the client (mysql vs pg), but I wanted two discrete, 'copy-and-paste' samples for both SQL options.

I still think you only need one connection method. Just set client in the Knex options to process.env.SQL_CLIENT, then the docs just need to tell the user to set the SQL_CLIENT env var.

ace-n · 2017-05-24T23:36:50Z

FYI, the "samples test app" failure is likely due to some configuration (or configuration-processing) issue. @jmdobry see trace below, or let me know if you want me to dig further into it.

➜  samples test app              
app: Starting app in: /Users/anassri/Desktop/nodejs-docs-samples/appengine/cloudsql
app: Using port: 45032
app: Running: node app.js
app: Child process ID: 4443
module.js:471
    throw err;
    ^

Error: Cannot find module '/Users/anassri/Desktop/nodejs-docs-samples/appengine/cloudsql/app.js'
    at Function.Module._resolveFilename (module.js:469:15)
    at Function.Module._load (module.js:417:25)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3
/Users/anassri/.nvm/versions/node/v6.10.0/lib/node_modules/@google-cloud/nodejs-repo-tools/src/utils/index.js:121
    if (arg.red) {
           ^

TypeError: Cannot read property 'red' of null
    at console.error.args.map (/Users/anassri/.nvm/versions/node/v6.10.0/lib/node_modules/@google-cloud/nodejs-repo-tools/src/utils/index.js:121:12)
    at Array.map (native)
    at Object.exports.error (/Users/anassri/.nvm/versions/node/v6.10.0/lib/node_modules/@google-cloud/nodejs-repo-tools/src/utils/index.js:120:94)
    at ChildProcess.childProcess.spawn.on (/Users/anassri/.nvm/versions/node/v6.10.0/lib/node_modules/@google-cloud/nodejs-repo-tools/src/cli/commands/test_commands/app.js:142:17)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)

jmdobry · 2017-05-25T17:06:44Z

appengine/cloudsql/server.js

 const crypto = require('crypto');

 const app = express();
 app.enable('trust proxy');
 // [END setup]
+let knex;
+
+function connectMysql () {


I still think you only need one connection method. Just set client in the Knex options to process.env.SQL_CLIENT, then the docs just need to tell the user to set the SQL_CLIENT env var.

jmdobry · 2017-05-25T17:07:17Z

appengine/cloudsql/README.md

-          mysql> grant all on YOUR_DATABASE.* to 'YOUR_USER'@'%';
+        mysql> create database `YOUR_DATABASE`;
+        mysql> create user 'YOUR_USER'@'%' identified by 'PASSWORD';
+        mysql> grant all on YOUR_DATABASE.* to 'YOUR_USER'@'%';

 1. Set the `MYSQL_USER`, `MYSQL_PASSWORD`, and `MYSQL_DATABASE` environment


These env vars need to be updated to the generic ones.

jmdobry · 2017-05-25T17:07:21Z

appengine/cloudsql/README.md

-1. Update the values in in `app.yaml` with your instance configuration.
+        export POSTGRES_USER="..."
+        export POSTGRES_PASSWORD="..."
+        export POSTGRES_DATABASE="..."


These env vars need to be updated to the generic ones.

jmdobry · 2017-05-25T17:07:34Z

appengine/cloudsql/README.md

+
+        export MYSQL_USER="..."
+        export MYSQL_PASSWORD="..."
+        export MYSQL_DATABASE="..."


These env vars need to be updated to the generic ones.

jmdobry · 2017-05-25T17:07:37Z

appengine/cloudsql/README.md

+Then, create a user (using the button in the *Access Control* > *Users* tab) and a
+database (using the button in the *Databases* tab).
+
+1. Set the `POSTGRES_USER`, `POSTGRES_PASSWORD`, and `POSTGRES_DATABASE` environment


These env vars need to be updated to the generic ones.

jmdobry · 2017-05-25T17:10:43Z

appengine/cloudsql/README.md

-          mysql> grant all on YOUR_DATABASE.* to 'YOUR_USER'@'%';
+        mysql> create database `YOUR_DATABASE`;
+        mysql> create user 'YOUR_USER'@'%' identified by 'PASSWORD';
+        mysql> grant all on YOUR_DATABASE.* to 'YOUR_USER'@'%';


Is this necessary still? The postgres docs below indicate this can all be done with the Cloud Console.

jmdobry · 2017-05-25T17:11:39Z

appengine/cloudsql/createTables.js

-    console.log(result);
+      // Connect to the database
+      return resolve(Knex({
+        client: 'mysql',


Again, I think we only need one connection method, just set the client Knex option to process.env.SQL_CLIENT.

jmdobry · 2017-05-25T17:12:03Z

appengine/cloudsql/createTables.js

+  // [END postgresMain]
+};
+
+exports.mysqlMain = function () {


Only need one main function

jmdobry · 2017-05-25T17:12:31Z

appengine/cloudsql/createTables.js

+
+// Get type of SQL client to use
+const sqlClient = process.env.SQL_CLIENT;
+if (sqlClient === 'pg') {


no need for conditional if we just rely on passing process.env.SQL_CLIENT to the Knex constructor

jmdobry · 2017-05-25T17:13:11Z

appengine/cloudsql/server.js

-  // [END listen]
+// Get type of SQL client to use
+const sqlClient = process.env.SQL_CLIENT;
+if (sqlClient === 'pg') {


No need for conditional if we rely on passing process.env.SQL_CLIENT to the Knex constructor

jmdobry · 2017-05-25T17:14:36Z

re: test error - What version of repo tools do you have?

jmdobry · 2017-05-25T17:16:57Z

appengine/cloudsql/package.json

-      "deploy": {
-        "substitutions": "YOUR_USER=$MYSQL_USER,YOUR_PASSWORD=$MYSQL_PASSWORD,YOUR_DATABASE=$MYSQL_DATABASE,YOUR_INSTANCE_CONNECTION_NAME=$INSTANCE_CONNECTION_NAME"
+        "substitutions": "YOUR_SQL_CLIENT=$SQL_CLIENT,YOUR_USER=$SQL_USER,YOUR_PASSWORD=$SQL_PASSWORD,YOUR_DATABASE=$SQL_DATABASE,YOUR_INSTANCE_CONNECTION_NAME=$INSTANCE_CONNECTION_NAME",
+        "startArgs": [


s/startArgs/args/

jmdobry

I think yarn needs to get run again to update the yarn.lock file. Do you have the latest version of yarn?

jmdobry · 2017-05-25T17:22:31Z

appengine/cloudsql/package.json

@@ -35,19 +35,18 @@
    "test": {


Missing a dependency on pg. Run yarn add pg

Missing also a dependency on knex

jmdobry · 2017-05-25T17:25:05Z

appengine/cloudsql/package.json

@@ -35,19 +35,18 @@
    "test": {


Missing also a dependency on knex

jmdobry · 2017-05-25T17:29:43Z

appengine/cloudsql/server.js

+    database: process.env.SQL_DATABASE
+  };
+
+  if (process.env.INSTANCE_CONNECTION_NAME) {


I think we should change to:

if (process.env.INSTANCE_CONNECTION_NAME && process.env.NODE_ENV === 'prod') {

Should this be prod, or production?

You're right, should be production

Add README Address comments; 'samples test app' still fails Address comments + make tests pass Enable Cloud SQL tests.

BREAKING CHANGE: The library now supports Node.js v10+. The last version to support Node.js v8 is tagged legacy-8 on NPM. New feature: methods with pagination now support async iteration.

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

Co-authored-by: Justin Beckwith <justin.beckwith@gmail.com>

ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 20, 2017

ace-n requested review from beccasaurus and jmdobry May 20, 2017 07:17

jmdobry requested changes May 22, 2017

View reviewed changes

jmdobry requested changes May 25, 2017

View reviewed changes

appengine/cloudsql/package.json

@@ -35,19 +35,18 @@

"test": {

Copy link

Member

jmdobry May 25, 2017

Choose a reason for hiding this comment

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

Missing also a dependency on knex

jmdobry requested changes May 25, 2017

View reviewed changes

First draft of postgres without the git mistake

5880a0c

Add README Address comments; 'samples test app' still fails Address comments + make tests pass Enable Cloud SQL tests.

jmdobry force-pushed the add-postgres-2 branch from d4c49d5 to 5880a0c Compare May 25, 2017 22:48

jmdobry approved these changes May 25, 2017

View reviewed changes

beccasaurus approved these changes May 26, 2017

View reviewed changes

ace-n merged commit 17f71e0 into master May 26, 2017

ace-n deleted the add-postgres-2 branch June 30, 2017 19:23

ahrarmonsur mentioned this pull request Nov 8, 2022

samples: migrate code from googleapis/nodejs-monitoring #2807

Merged

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.7.1 (#396)

6ea9d7b

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.7.1 (#396)

5009822

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.7.1 (#396)

896e9f9

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.7.1 (#396)

31f7a45

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

ahrarmonsur pushed a commit that referenced this pull request Nov 16, 2022

chore: release 3.7.1 (#396)

cec2937

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

ahrarmonsur mentioned this pull request Nov 16, 2022

migrate code from googleapis/nodejs-asset #2856

Merged

ace-n pushed a commit that referenced this pull request Nov 17, 2022

chore: release 3.7.1 (#396)

86f9508

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

NimJay mentioned this pull request Nov 19, 2022

migrate code from googleapis/nodejs-dataproc #2878

Merged

NimJay pushed a commit that referenced this pull request Nov 19, 2022

chore(samples): no native sleep (#396)

bc33ba3

Co-authored-by: Justin Beckwith <justin.beckwith@gmail.com>

NimJay pushed a commit that referenced this pull request Nov 19, 2022

chore(samples): no native sleep (#396)

1ed3cad

Co-authored-by: Justin Beckwith <justin.beckwith@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add postgres samples #396

Add postgres samples #396

ace-n commented May 20, 2017

codecov-io commented May 20, 2017 •

edited

Loading

jmdobry left a comment •

edited

Loading

jmdobry May 22, 2017

ace-n May 24, 2017

jmdobry May 22, 2017

ace-n May 24, 2017

jmdobry May 25, 2017

ace-n commented May 24, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry commented May 25, 2017

jmdobry May 25, 2017

jmdobry left a comment

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

jmdobry May 25, 2017

ace-n May 25, 2017

jmdobry May 25, 2017

Add postgres samples #396

Add postgres samples #396

Conversation

ace-n commented May 20, 2017

codecov-io commented May 20, 2017 • edited Loading

Codecov Report

jmdobry left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmdobry commented May 25, 2017

Choose a reason for hiding this comment

jmdobry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented May 20, 2017 •

edited

Loading

jmdobry left a comment •

edited

Loading