Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

grayside · 2019-08-22T20:04:38Z

This PR is intended as a spot for some broad improvements to the ImageMagick sample after feedback in the duplicated code in #1445. Once this PR is in, the code in the other can be updated. We can knock some stuff around but let's focus on getting something ready with a ~1 week max turnaround.

Changes

test/index.test.js

Only clean up blurred image if it exists
Only run tests if source files exist in Cloud Storage.

index.js

Promise.reject() => throw new Error
Re-order require statements
Use stable/default version of the client library

Other Feedback Not Yet Incorporated

Promisify gm. Unclear how to do this, Promisify gm methods? aheckmann/gm#320 leads to the current implementation.
data/object naming: data is the standard function signature in the Function Framework, object reassignment is to align with the idea of a Cloud Storage object. I defer to Ace on this.

Other feedback for discussion

BLURRED_BUCKET_NAME is a global variable, why pass it in as a function parameter? Yes it is, do we prefer to access the global variable generally or try to limit functions to their parameter signature where possible?

grant

Looks good.
Added a few comments that should be addressed before merging.

functions/imagemagick/index.js

grant · 2019-08-22T20:39:04Z

functions/imagemagick/index.js

@@ -71,8 +72,7 @@ const blurImage = async (file, blurredBucketName) => {

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


Are we sure this will stringify correctly? Do we not mean to provide the ${err.message}?

functions/imagemagick/test/index.test.js

grant · 2019-08-22T20:54:40Z

functions/imagemagick/test/index.test.js


 const safeFileName = 'bicycle.jpg';
 const offensiveFileName = 'zombie.jpg';
-
+const BUCKET_NAME = process.env.FUNCTIONS_BUCKET;


Nit: It would be nice if we just used FUNCTIONS_BUCKET so that we could destruct process.env in the same way:

FUNCTIONS_BUCKET

BLURRED_FUNCTIONS_BUCKET

Agreed that it's a bit confusing. From what I can see, BUCKET_NAME is used in the same because it makes sense as simply the bucket causing the function trigger. FUNCTIONS_BUCKET is defined in .kokoro/build.sh and has that name so the product testing config is disambiguated.

grant · 2019-08-22T20:55:45Z

functions/imagemagick/index.js

@@ -98,8 +98,7 @@ const blurImage = async (file, blurredBucketName) => {
    await blurredBucket.upload(tempLocalPath, {destination: file.name});
    console.log(`Uploaded blurred image to: ${gcsPath}`);
  } catch (err) {
-    console.error(`Unable to upload blurred image to ${gcsPath}:`, err);
-    return Promise.reject(err);
+    throw new Error(`Unable to upload blurred image to ${gcsPath}: ${err}`);


Same comment about err.message.

grayside · 2019-08-26T16:31:10Z

The test failures are:

missing write permission which seems unrelated to this change:

npm WARN checkPermissions Missing write access to /usr/local/lib/node_modules
npm ERR! path /usr/local/lib/node_modules
npm ERR! code EACCES
npm ERR! errno -13
npm ERR! syscall access
npm ERR! Error: EACCES: permission denied, access '/usr/local/lib/node_modules'
npm ERR!  { Error: EACCES: permission denied, access '/usr/local/lib/node_modules'
npm ERR!   stack: 'Error: EACCES: permission denied, access \'/usr/local/lib/node_modules\'',
npm ERR!   errno: -13,
npm ERR!   code: 'EACCES',
npm ERR!   syscall: 'access',
npm ERR!   path: '/usr/local/lib/node_modules' }

I added a validation check that the zombie.jpg and bicycle.png files are available in the source bucket. It turns out that bicycle.png does not exist. The reason the test for "safeFileName" was passing is that the function returns a 204 if the file does not exist.

I am looking into (2).

googlebot · 2019-08-26T17:02:33Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

grayside · 2019-08-26T17:04:11Z

Tweaked how files in GCS are checked to better support await and iteration.
Added a separate test case for a missing file.

googlebot · 2019-08-26T17:04:53Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

grayside · 2019-08-27T19:47:30Z

Ace, this PR is currently blocked on your intentions for the bicycle.png image.

ace-n · 2019-08-27T21:04:30Z

functions/imagemagick/test/index.test.js

+describe('functions/imagemagick tests', async () => {
+  let startFF, stopFF;
+  // Successfully generated images require cleanup.
+  let cleanupRequired = false;


Why not just attempt to delete the file, and swallow the error if (and only if) it doesn't exist?

(IIRC that's more in-line with what our other samples do.)

I bullet-proofed what I found instead of removing it. I can take this approach.

googlebot · 2019-08-27T22:41:49Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-27T22:43:11Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

functions/imagemagick: robustness and idiomatic improvements

054c585

grayside requested review from fhinkel and ace-n August 22, 2019 20:04

grayside requested a review from grant as a code owner August 22, 2019 20:04

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

grant approved these changes Aug 22, 2019

View reviewed changes

grayside added 3 commits August 22, 2019 14:38

functions/imagemagick: lint fixes

aa5551c

functions/imagemagick: reflow require statements to group storage

6f57503

functions/imagemagick: nodejs style and comment feedback

8bb8a01

functions/imagemagick: improve test coverage, add missing file test

1011648

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 26, 2019

grayside added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 26, 2019

grayside assigned ace-n Aug 27, 2019

ace-n approved these changes Aug 27, 2019

View reviewed changes

grayside self-assigned this Aug 27, 2019

functions/imagemagick: allow cleanup to fail gracefully

a08a2dd

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 27, 2019

grayside added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 27, 2019

fhinkel approved these changes Aug 28, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

grayside commented Aug 22, 2019

grant left a comment

grant Aug 22, 2019

grant Aug 22, 2019

grayside Aug 22, 2019 •

edited

Loading

grant Aug 22, 2019

grayside commented Aug 26, 2019

googlebot commented Aug 26, 2019

grayside commented Aug 26, 2019

googlebot commented Aug 26, 2019

grayside commented Aug 27, 2019

ace-n Aug 27, 2019

grayside Aug 27, 2019

googlebot commented Aug 27, 2019

googlebot commented Aug 27, 2019

Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

Conversation

grayside commented Aug 22, 2019

Changes

test/index.test.js

index.js

Other Feedback Not Yet Incorporated

Other feedback for discussion

grant left a comment

Choose a reason for hiding this comment

grant Aug 22, 2019

Choose a reason for hiding this comment

grant Aug 22, 2019

Choose a reason for hiding this comment

grayside Aug 22, 2019 • edited Loading

Choose a reason for hiding this comment

grant Aug 22, 2019

Choose a reason for hiding this comment

grayside commented Aug 26, 2019

googlebot commented Aug 26, 2019

grayside commented Aug 26, 2019

googlebot commented Aug 26, 2019

grayside commented Aug 27, 2019

ace-n Aug 27, 2019

Choose a reason for hiding this comment

grayside Aug 27, 2019

Choose a reason for hiding this comment

googlebot commented Aug 27, 2019

googlebot commented Aug 27, 2019

grayside Aug 22, 2019 •

edited

Loading