run/image-processing: add sample #1445

grayside · 2019-08-09T21:58:00Z

This is a new sample demonstrating how Cloud Run can be triggered by Pub/Sub to perform work with GCS for storage, Cloud Vision integration, and command-line utilities.

A few considerations:

This sample explicitly copies in the functions/imagemagick/index.js. It is oriented this way in the notion of directly sharing the code, but the current implementation does this using manual copy operations.
I've applied functions/imagemagick: fix "callback must be a function" error #1444 to the copied code in this PR.
Because of the copying behavior, I've not yet committed to any kind of test coverage for the logic covered in the GCF tests.

Reviewer Actions

In addition to the normal review of this sample, I would like feedback on whether a more automated approach to code sharing would be reasonable at this time. I anticipate more of this kind of sharing and cross-product reuse to come up, especially as Function Frameworks grow.

fhinkel: Code sharing general feedback
grant: Cloud Run sample review
ace: imagemagick sharing/usage

Node.js practices by anyone.

run/image-processing/.env

run/image-processing/README.md

run/image-processing/app.js

fhinkel · 2019-08-13T20:03:58Z

run/image-processing/image.js

+const path = require('path');
+const {Storage} = require('@google-cloud/storage');
+const storage = new Storage();
+const vision = require('@google-cloud/vision').v1p1beta1;


Any reason we can't use the default version?

fhinkel · 2019-08-13T20:04:22Z

run/image-processing/image.js

+const {promisify} = require('util');
+const path = require('path');
+const {Storage} = require('@google-cloud/storage');
+const storage = new Storage();


I'd list all requires before other variables.

Per my PR notes, the image.js file is literally functions/imagemagick/index.js, copied over and with region tags replaced. My goal is not to fork it, hence my previous proposal of uniting them around a code templating tool. Can we move this feedback to a separate issue to the GCF code?

(And of course, feedback welcome on how to approach this idea of shared code across samples)

fhinkel · 2019-08-13T20:11:55Z

run/image-processing/image.js

+// [START run_imageproc_handler_analyze]
+// Blurs uploaded images that are flagged as Adult or Violence.
+exports.blurOffensiveImages = async event => {
+  // This event represents the triggering Cloud Storage object.


Why not use object in the function signature instead of reassigning it? Looking at the call site, we use data. Maybe change the parameter to data or imageData?

Maybe data isn't the greatest name. But I'd not re-assign immediately.

fhinkel

lgtm, thanks!

Sometimes, I find it more intuitive to read

async foo () => {
  await a();
  const res = await b();
  return res;
}

but the following is the same:

async foo () => {
  await a();
  return b();
}

fhinkel · 2019-08-13T20:14:17Z

run/image-processing/image.js

+// [START run_imageproc_handler_analyze]
+// Blurs uploaded images that are flagged as Adult or Violence.
+exports.blurOffensiveImages = async event => {
+  // This event represents the triggering Cloud Storage object.


Maybe data isn't the greatest name. But I'd not re-assign immediately.

fhinkel · 2019-08-13T20:15:25Z

run/image-processing/image.js

+    const detections = result.safeSearchAnnotation || {};
+
+    if (
+      detections.adult === 'VERY_LIKELY' ||


Do you want to add a comment here what the other levels are?

fhinkel · 2019-08-13T20:26:29Z

run/image-processing/image.js

+    }
+  } catch (err) {
+    console.error(`Failed to analyze ${file.name}.`, err);
+    return Promise.reject(err);


Usually you don't want to manually create promises when you have async/await. It's more idiomatic with async/await to throw instead.

} catch (err) { console.error(`Failed to analyze ${file.name}.`, err); throw err; // Or throw new Error(`Failed to analyze ${file.name}.: ${err}`); }

fhinkel · 2019-08-13T20:27:34Z

run/image-processing/image.js

+
+// [START run_imageproc_handler_blur]
+// Blurs the given file using ImageMagick, and uploads it to another bucket.
+const blurImage = async (file, blurredBucketName) => {


Don't you have Blurred_bucket_name as a global variable? Sorry, might be missing it in the GitHub view.

fhinkel · 2019-08-13T20:28:00Z

run/image-processing/image.js

+    console.log(`Downloaded ${file.name} to ${tempLocalPath}.`);
+  } catch (err) {
+    console.error('File download failed.', err);
+    return Promise.reject(err);


throw instead of promise

fhinkel · 2019-08-13T20:30:04Z

run/image-processing/image.js

+  }
+
+  await new Promise((resolve, reject) => {
+    gm(tempLocalPath)


can you use promisify on gm?

fhinkel · 2019-08-13T20:36:41Z

run/image-processing/package.json

@@ -0,0 +1,31 @@
+{
+  "name": "nodejs-image-processing",
+  "version": "1.0.0",


I think if the package is private, you don't need a version. Private means it can't be published.

I think it's important for developers to think about version numbers for all projects even if not publishing, but I can't say I thought about this one. As a practice what do we prefer to do in samples on version and private?

I agree that version numbers are important. In our samples though we never paid much attention to them and I'd prefer not having them than having meaningless numbers.

fhinkel · 2019-08-13T20:51:37Z

Code sharing general feedback

I'd say for samples, copy and paste duplication is better than a special setup for the sake of DRY.

Pros:

For automated syntax updates, it doesn't matter if the tool updates one or two files.
If something breaks, CI will alert us in both places and it's not that big a deal to fix it twice.
When it's time to archive a sample, we can easily do so without figuring out if other places rely on the code.
Clear ownership, self contained sample.

Cons:

I just reviewed code that's already checked in. That was unnecessary - or somebody should make the same improvements to the other sample. But then the whole sample might need changes to fit the style. 🤔

cc @bcoe if they have thoughts

bcoe

I don't love that we end up with identical files gradually diverging, although I do like that we have folders of code that work in isolation. Also, I imagine tutorials will potentially diverge more over time.

I think this is a wider discussion for outside this PR however 😄 how can we DRY up code when possible, e.g., if the same Node.js docker setup is used in 5 tutorials, while still having a repo that someone can just clone and run.

bcoe · 2019-08-14T16:25:41Z

run/image-processing/Dockerfile

+# Install Imagemagick into the container image.
+# For more on system packages review the system packages tutorial.
+# https://cloud.google.com/run/docs/tutorials/system-packages#dockerfile
+RUN set -ex; \


It sounds like we use imagemagick in a few other samples? Just wondering about the ImageMagick license type, mainly.

We also use it in functions/imagemagick, from which the image.js file in this sample is copied. It is also included in App Engine and Cloud Functions runtimes.

run/image-processing/README.md

run/image-processing/app.js

grayside · 2019-08-22T17:33:30Z

I am now looking at applying the feedback here for image.js to functions/imagemagick/index.js to decouple the improvements.

fhinkel · 2019-08-28T01:56:02Z

Should I go ahead and merge this? I suggest we duplicate the code for now and merge if we have a discussion and decide that we don't like the code duplication. Or would it be easier to start with shared resources and copy if we prefer simplicity over DRY?

…1456) * functions/imagemagick: robustness and idiomatic improvements * functions/imagemagick: lint fixes * functions/imagemagick: reflow require statements to group storage * functions/imagemagick: nodejs style and comment feedback * functions/imagemagick: improve test coverage, add missing file test * functions/imagemagick: allow cleanup to fail gracefully

grayside · 2019-08-28T19:16:03Z

@fhinkel now that the functions/imagemagick code is updated, I've synced the code here following the instructions at the bottom of the README. I think this is ready for merge.

run/image-processing: add sample

6b85616

grayside requested review from grant and ace-n August 9, 2019 21:58

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 9, 2019

grayside changed the title ~~run/image-processing: add sample~~ run/image-processing: add sample [NO MERGE] Aug 9, 2019

grayside requested a review from fhinkel August 9, 2019 21:58

run/imageproc: lint fixes

cf12943