Migrate Sample: Cloud Run as Pub/Sub Push Handler#1419
Conversation
googlebot
added
the
cla: yes
|
The test output from this sample is currently impacted by GoogleCloudPlatform/nodejs-repo-tools#214 |
ace-n
left a comment
ace-n
left a comment
There was a problem hiding this comment.
LGTM other than a nit or two. 🙂
run/pubsub/app.js
Outdated
| // license that can be found in the LICENSE file. | ||
|
|
||
| // [START run_pubsub_server_setup] | ||
|
|
There was a problem hiding this comment.
Nit: extra space in region tag area?
There was a problem hiding this comment.
Ah, go standards transference. Fixing.
| app.post('/', (req, res) => { | ||
| if (!req.body) { | ||
| const msg = 'no Pub/Sub message received'; | ||
| console.error(`error: ${msg}`); |
There was a problem hiding this comment.
Nit: in an ideal world, lines 19-20 wouldn't be repeated in the clause below (lines 26-27) - but I don't think there's a good way to avoid this repetition here. 🙁
run/pubsub/test/app.test.js
Outdated
| const uuid = require('uuid'); | ||
|
|
||
| const cwd = path.join(__dirname, '../'); | ||
| const requestObj = tools.getRequest({cwd}); |
There was a problem hiding this comment.
@fhinkel this uses nodejs-repo-tools - I assume that's OK for this sample, since it's not a simple hello-world?
There was a problem hiding this comment.
We're trying to remove the usage of repo-tools. Can you use request or fetch instead of tools.getRequest?
There was a problem hiding this comment.
You can have a look at the appengine Hello World example, we use supertest for this kind of testing. Since we're not running e2e tests there's no need to install repo-tools. But not blocking, I'll work on removing repo-tools from the samples anyways.
There was a problem hiding this comment.
Switched to sinon and supertest.
fhinkel
left a comment
fhinkel
left a comment
There was a problem hiding this comment.
Please don't commit package-lock.json. Otherwise LGTM
|
@fhinkel why not commit package-lock.json? Without that the builds are not deterministic and I need to switch the Dockerfile back to npm install instead of following the guidance on https://nodejs.org/de/docs/guides/nodejs-docker-webapp/ to steer a production-centric build to use npm ci. |
|
Not sure why the test is failing. The build log indicates it should be passing |
|
In production you need Lock files other cause merge conflicts and I see to benefit checking them in for samples. If we decide to change this, we should do it consistently for all samples and not just this one. |
|
Failing tests see #1435 |
|
Tests passed |
|
@fhinkel looks like I've addressed all current concerns, could you approve or clarify remaining issues? |
kokoro-team
removed
the
kokoro:run
|
@grayside |
|
Landing and making |
5 participants
This sample has been previously reviewed in previous source control. Changes include updating dependencies and switching from ava to mocha.
This sample is currently in use in http://cloud.google.com/run/docs/tutorials/pubsub