Merge samples for Flex and Std #590

michaelawyu · 2018-03-30T08:41:00Z

No description provided.

…s and solve conflicts

ace-n · 2018-03-30T19:22:07Z

appengine/analytics/README.md


-With `npm`:
+    gcloud app deploy app.standard.yaml


Nit - these indentations are different.

ace-n · 2018-03-30T19:25:13Z

appengine/analytics/package.json

@@ -10,16 +10,14 @@
    "url": "https://github.com/GoogleCloudPlatform/nodejs-docs-samples.git"
  },
  "engines": {
-    "node": ">=4"


@jmdobry @lukesneeringer Which versions of Node do we support? AFAIK, this is too restrictive (at least for Flex).

@michaelawyu if you make any changes as a result of this comment, please apply those changes to any other package.json files that have this issue.

Node 4+, until April 30 when Node 4 reaches end of life and we can switch our samples to support Node 6+

ace-n · 2018-03-30T19:26:37Z

appengine/analytics/package.json

    "start": "node app.js",
    "lint": "repo-tools lint",
    "pretest": "npm run lint",
    "system-test": "repo-tools test app",
-    "test": "npm run system-test",
-    "e2e-test": "repo-tools test deploy"


Why is this removed? (Perhaps because npm run deploy no longer works?)

Yeah, it's removed because npm run deploy no longer works.

ace-n · 2018-03-30T19:28:26Z

appengine/building-an-app/build/package.json

+  "private": true,
+  "main": "server.js",
+  "scripts": {
+    "lint": "samples lint",


In README.md, you specify start and deploy scripts. Make sure to add those (and their respective commands) here, or remove them from your README.md.

ace-n · 2018-03-30T19:31:05Z

appengine/building-an-app/build/test/server.test.js

+test.beforeEach(utils.stubConsole);
+test.afterEach.always(utils.restoreConsole);
+
+test.cb.serial(`should send greetings`, (t) => {


@jmdobry what are your thoughts on using repo-tools instead of this test file?

If we can still do the same tests while deleting code, let's do it.

Tests for building-an-app/build/ are now done by repo-tools.

ace-n · 2018-03-30T20:22:34Z

appengine/metadata/standard/server.js

+const app = express();
+app.enable('trust proxy');
+
+const METADATA_NETWORK_INTERFACE_URL = 'http://metadata/computeMetadata/v1/' +


Nit: combine this into one string (and ignore line length limits, or split onto multiple lines using backticks)?

ace-n · 2018-03-30T20:23:12Z

appengine/metadata/standard/server.js

+  return request(METADATA_NETWORK_INTERFACE_URL, options)
+    .then((response) => response.body)
+    .catch((err) => {
+      if (err || err.statusCode !== 200) {


Did you mean && instead of ||?

ace-n · 2018-03-30T20:27:51Z

appengine/redis/README.md

 app.

-You can also read the [node_redis documentation][3].


Typo (missing an i): addit-i-onally

ace-n · 2018-03-30T20:29:04Z

appengine/sendgrid/app.standard.yaml

+
+# [START env_variables]
+env_variables:
+  SENDGRID_API_KEY: <your-sendgrid-api-key>


Ditto to "do not commit this file" comment

ace-n · 2018-03-30T20:30:25Z

appengine/storage/standard/README.md

@@ -0,0 +1,40 @@
+# Node.js Google Cloud Storage sample for Google App Engine


Does this need to be split into separate standard and flexible samples?

This one is split as App Engine Node.js Std does not support the resumable upload feature in Cloud Storage client library. Should we use the sample w/ resumable upload disabled for both std and flex?

@frankyn WDYT?

codecov · 2018-04-02T21:49:06Z

Codecov Report

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

@@          Coverage Diff          @@
##           master   #590   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines           3      3           
=====================================
  Hits            3      3

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 daf141d...2fd9d85. Read the comment docs.

michaelawyu · 2018-04-09T21:17:41Z

@lesv

ace-n

LGTM once you switch the appropriate tests to using @google-cloud/repo-tools. (Ping @jmdobry or myself if you want us to give you an intro on that library.)

ace-n · 2018-04-09T22:13:20Z

appengine/cloudsql_postgresql/README.md

@@ -1,15 +1,17 @@
-# Cloud SQL for MySQL Node.js sample on App Engine flexible environment
+# Cloud SQL for PostgresSQL Node.js sample on App Engine flexible environment


Nit: IIRC, it's PostgreSQL (extra S).

Me and my fat fingers :( It's been fixed

lesv · 2018-04-09T23:23:34Z

@michaelawyu I see places where you use app.flexible.yaml and other places where you use /flexible/app.yaml -- perhaps we should be consistent?

michaelawyu · 2018-04-10T00:04:36Z

Hi @lesv ! It is intended actually: samples that share the same code between Flex and Std are merged together and have two .YAML files; samples that use different code for different environments are kept separate and have one .YAML file in each folder; getting-started samples are always kept separate so that beginners can use gcloud app deploy to deploy the sample without knowing the differences between Flex and Std.

lesv · 2018-04-10T00:43:57Z

SGTM

RichieEscarez · 2018-04-10T01:40:49Z

General comment:

When referring to an "environment" of App Engine, it should be lowercase, either:

"App Engine standard environment"
"standard environment"
"App Engine flexible environment"
"flexible environment"

(with the exceptions of either a title or the start of a sentence, in which case you should capitalize the first letter).

RichieEscarez · 2018-04-10T01:50:17Z

appengine/sendgrid/README.md

@@ -16,7 +18,8 @@ Before you can run or deploy the sample, you will need to do the following:
 1. [Create a SendGrid Account](http://sendgrid.com/partner/google). As of
 September 2015, Google users start with 25,000 free emails per month.
 1. Configure your SendGrid settings in the environment variables section in
-`app.yaml`.
+`app.standard.yaml` (if you are deploying to App Engine Standard Environment) or


The inline code comment in the /sendgrid/app.js file refers to "app.yaml" which could be confusing now that we have these two environment specific files, which are named differently.

To assist with learning this concept of "uniquely-named, service-specific app.yaml files", perhaps add to the "app.js" file:
(for example, app.standard.yaml or app.flexible.yaml)

-> therefore you instead have:

"....variables are set by app.yaml (for example, app.standard.yaml or app.flexible.yaml) when running on ......"

ace-n · 2018-04-11T21:45:05Z

LGTM - @michaelawyu feel free to merge once @RichieEscarez (and maybe @lesv) also approve(s).

lesv · 2018-04-11T21:47:36Z

I just had a nit.

lesv · 2018-04-11T21:47:51Z

So LGTM from me.

* chore(main): release 4.0.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Merge samples for Flex and Std

e42d222

michaelawyu requested a review from ace-n March 30, 2018 08:41

michaelawyu added 2 commits March 30, 2018 01:47

Solve conflicts

63f087d

Merge remote-tracking branch 'upstream/master' into app-engine-sample…

ebccfbb

…s and solve conflicts

ace-n suggested changes Mar 30, 2018

View reviewed changes

MInor Fixes

905d597

michaelawyu added 2 commits April 2, 2018 14:49

More minor fixes

79fca64

Lint

c0de187

ace-n suggested changes Apr 9, 2018

View reviewed changes

RichieEscarez reviewed Apr 10, 2018

View reviewed changes

Applied requested changes

787fd0b

ace-n approved these changes Apr 11, 2018

View reviewed changes

chenyumic added 2 commits April 11, 2018 14:51

Merge branch 'master' into app-engine-samples

63377bc

Merge branch 'master' into app-engine-samples

2fd9d85

michaelawyu merged commit c11163d into GoogleCloudPlatform:master Apr 11, 2018

This was referenced Dec 23, 2020

[Snyk] Security upgrade google-auth-library from 2.0.2 to 3.0.0 leejh3224/nodejs-docs-samples#287

Open

[Snyk] Fix for 1 vulnerabilities leejh3224/nodejs-docs-samples#293

Open

This was referenced Sep 5, 2021

[Snyk] Fix for 1 vulnerabilities leejh3224/nodejs-docs-samples#349

Open

[Snyk] Security upgrade google-auth-library from 2.0.2 to 3.0.0 leejh3224/nodejs-docs-samples#354

Open

This was referenced Jan 12, 2022

[Snyk] Fix for 1 vulnerabilities leejh3224/nodejs-docs-samples#423

Open

[Snyk] Security upgrade google-auth-library from 2.0.2 to 3.0.0 leejh3224/nodejs-docs-samples#426

Open

This was referenced Feb 10, 2022

[Snyk] Security upgrade google-auth-library from 2.0.2 to 3.0.0 leejh3224/nodejs-docs-samples#456

Open

[Snyk] Fix for 1 vulnerabilities leejh3224/nodejs-docs-samples#464

Open

NimJay mentioned this pull request Nov 19, 2022

migrate code from googleapis/nodejs-dataproc #2878

Merged

		@@ -0,0 +1,40 @@
		# Node.js Google Cloud Storage sample for Google App Engine

		@@ -1,15 +1,17 @@
		# Cloud SQL for MySQL Node.js sample on App Engine flexible environment
		# Cloud SQL for PostgresSQL Node.js sample on App Engine flexible environment

Merge samples for Flex and Std #590

Merge samples for Flex and Std #590

Conversation

michaelawyu commented Mar 30, 2018

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

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

Choose a reason for hiding this comment

codecov bot commented Apr 2, 2018 • edited Loading

Codecov Report

michaelawyu commented Apr 9, 2018

ace-n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lesv commented Apr 9, 2018

michaelawyu commented Apr 10, 2018

lesv commented Apr 10, 2018

RichieEscarez commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ace-n commented Apr 11, 2018 • edited Loading

lesv commented Apr 11, 2018

lesv commented Apr 11, 2018

codecov bot commented Apr 2, 2018 •

edited

Loading

ace-n commented Apr 11, 2018 •

edited

Loading