Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

Merged
merged 7 commits into from
Aug 28, 2019
Merged

Functions: ImageMagick tutorial: Idiomatic & Robustness Improvements #1456

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
054c585
functions/imagemagick: robustness and idiomatic improvements
grayside Aug 22, 2019
aa5551c
functions/imagemagick: lint fixes
grayside Aug 22, 2019
6f57503
functions/imagemagick: reflow require statements to group storage
grayside Aug 26, 2019
8bb8a01
functions/imagemagick: nodejs style and comment feedback
grayside Aug 26, 2019
1011648
functions/imagemagick: improve test coverage, add missing file test
Aug 26, 2019
a08a2dd
functions/imagemagick: allow cleanup to fail gracefully
grayside Aug 27, 2019
39004fd
Merge branch 'master' into functions/imagemagick-polish
grayside Aug 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions functions/imagemagick/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
grayside marked this conversation as resolved.
Show resolved Hide resolved
const client = new vision.ImageAnnotatorClient();

const {BLURRED_BUCKET_NAME} = process.env;
Expand All @@ -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.
Show resolved Hide resolved
detections.adult === 'VERY_LIKELY' ||
detections.violence === 'VERY_LIKELY'
) {
Expand All @@ -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]
Expand All @@ -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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ${err.message}?

}

await new Promise((resolve, reject) => {
Expand All @@ -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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about err.message.

}

// Delete the temporary file.
Expand Down
88 changes: 57 additions & 31 deletions functions/imagemagick/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,40 +31,64 @@ 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@grayside grayside Aug 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The 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 .kokoro/build.sh and has that name so the product testing config is disambiguated.

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.
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;
}
};
});

before(async () => {
let exists = await storage
.bucket(BUCKET_NAME)
.file(offensiveFileName)
.exists();
if (!exists[0]) {
grayside marked this conversation as resolved.
Show resolved Hide resolved
throw Error(`Missing required file: gs://${BUCKET_NAME}/${offensiveFileName}`);
}

throw err;
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);
// beforeEach(tools.stubConsole);
grayside marked this conversation as resolved.
Show resolved Hide resolved
// afterEach(tools.restoreConsole);

it('blurOffensiveImages detects safe images using Cloud Vision', async () => {
const PORT = 8080;
Expand All @@ -81,7 +105,6 @@ describe('functions/imagemagick tests', () => {
});

const {stdout} = await stopFF(ffProc);

assert.ok(stdout.includes(`Detected ${safeFileName} as OK.`));
});

Expand All @@ -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) {
Expand Down