-
Notifications
You must be signed in to change notification settings - Fork 2k
Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
054c585
aa5551c
6f57503
8bb8a01
1011648
a08a2dd
39004fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,9 +21,9 @@ const fs = require('fs'); | |
| const {promisify} = require('util'); | ||
| const path = require('path'); | ||
| const {Storage} = require('@google-cloud/storage'); | ||
| const storage = new Storage(); | ||
| const vision = require('@google-cloud/vision').v1p1beta1; | ||
| const vision = require('@google-cloud/vision'); | ||
|
|
||
| const storage = new Storage(); | ||
| const client = new vision.ImageAnnotatorClient(); | ||
|
|
||
| const {BLURRED_BUCKET_NAME} = process.env; | ||
|
|
@@ -45,6 +45,7 @@ exports.blurOffensiveImages = async event => { | |
| const detections = result.safeSearchAnnotation || {}; | ||
|
|
||
| if ( | ||
| // Levels are defined in https://cloud.google.com/vision/docs/reference/rpc/google.cloud.vision.v1#google.cloud.vision.v1.Likelihood | ||
grayside marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| detections.adult === 'VERY_LIKELY' || | ||
| detections.violence === 'VERY_LIKELY' | ||
| ) { | ||
|
|
@@ -55,7 +56,7 @@ exports.blurOffensiveImages = async event => { | |
| } | ||
| } catch (err) { | ||
| console.error(`Failed to analyze ${file.name}.`, err); | ||
| return Promise.reject(err); | ||
| throw err; | ||
| } | ||
| }; | ||
| // [END functions_imagemagick_analyze] | ||
|
|
@@ -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}`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure this will stringify correctly? Do we not mean to provide the |
||
| } | ||
|
|
||
| await new Promise((resolve, reject) => { | ||
|
|
@@ -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}`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about |
||
| } | ||
|
|
||
| // Delete the temporary file. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,37 +31,61 @@ requestRetry = requestRetry.defaults({ | |
| retryDelay: 1000, | ||
| }); | ||
|
|
||
| const BUCKET_NAME = process.env.FUNCTIONS_BUCKET; | ||
| const {BLURRED_BUCKET_NAME} = process.env; | ||
|
|
||
| const safeFileName = 'bicycle.jpg'; | ||
| const offensiveFileName = 'zombie.jpg'; | ||
|
|
||
| const BUCKET_NAME = process.env.FUNCTIONS_BUCKET; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: It would be nice if we just used
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| const {BLURRED_BUCKET_NAME} = process.env; | ||
| const blurredBucket = storage.bucket(BLURRED_BUCKET_NAME); | ||
| const cwd = path.join(__dirname, '..'); | ||
|
|
||
| const blurredBucket = storage.bucket(BLURRED_BUCKET_NAME); | ||
| // Successfully generated images require cleanup. | ||
| let cleanupRequired = false; | ||
grayside marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| describe('functions/imagemagick tests', () => { | ||
| const startFF = port => { | ||
| return execPromise( | ||
| `functions-framework --target=blurOffensiveImages --signature-type=event --port=${port}`, | ||
| {timeout: 15000, shell: true, cwd} | ||
| ); | ||
| }; | ||
|
|
||
| const stopFF = async ffProc => { | ||
| try { | ||
| return await ffProc; | ||
| } catch (err) { | ||
| // Timeouts always cause errors on Linux, so catch them | ||
| if (err.name && err.name === 'ChildProcessError') { | ||
| const {stdout, stderr} = err; | ||
| return {stdout, stderr}; | ||
| let startFF, stopFF; | ||
|
|
||
| before(() => { | ||
| startFF = port => { | ||
| return execPromise( | ||
| `functions-framework --target=blurOffensiveImages --signature-type=event --port=${port}`, | ||
| {timeout: 15000, shell: true, cwd} | ||
| ); | ||
| }; | ||
|
|
||
| stopFF = async ffProc => { | ||
| try { | ||
| return await ffProc; | ||
| } catch (err) { | ||
| // Timeouts always cause errors on Linux, so catch them | ||
| if (err.name && err.name === 'ChildProcessError') { | ||
| const {stdout, stderr} = err; | ||
| return {stdout, stderr}; | ||
| } | ||
|
|
||
| throw err; | ||
| } | ||
| }; | ||
| }); | ||
|
|
||
| throw err; | ||
| before(async () => { | ||
| let exists = await storage | ||
| .bucket(BUCKET_NAME) | ||
| .file(offensiveFileName) | ||
| .exists(); | ||
| if (!exists[0]) { | ||
grayside marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| throw Error( | ||
| `Missing required file: gs://${BUCKET_NAME}/${offensiveFileName}` | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| exists = await storage | ||
| .bucket(BUCKET_NAME) | ||
| .file(safeFileName) | ||
| .exists(); | ||
| if (!exists[0]) { | ||
| throw Error(`Missing required file: gs://${BUCKET_NAME}/${safeFileName}`); | ||
| } | ||
| }); | ||
|
|
||
| beforeEach(tools.stubConsole); | ||
| afterEach(tools.restoreConsole); | ||
|
|
@@ -81,7 +105,6 @@ describe('functions/imagemagick tests', () => { | |
| }); | ||
|
|
||
| const {stdout} = await stopFF(ffProc); | ||
|
|
||
| assert.ok(stdout.includes(`Detected ${safeFileName} as OK.`)); | ||
| }); | ||
|
|
||
|
|
@@ -108,16 +131,19 @@ describe('functions/imagemagick tests', () => { | |
| ) | ||
| ); | ||
|
|
||
| assert.ok( | ||
| storage | ||
| .bucket(BLURRED_BUCKET_NAME) | ||
| .file(offensiveFileName) | ||
| .exists(), | ||
| 'File uploaded' | ||
| ); | ||
| const exists = storage | ||
| .bucket(BLURRED_BUCKET_NAME) | ||
| .file(offensiveFileName) | ||
| .exists(); | ||
|
|
||
| assert.ok(exists, 'File uploaded'); | ||
| cleanupRequired |= exists; | ||
| }); | ||
|
|
||
| after(async () => { | ||
| if (!cleanupRequired) { | ||
| return; | ||
| } | ||
| try { | ||
| await blurredBucket.file(offensiveFileName).delete(); | ||
| } catch (err) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.