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

Marc's final feedback on CUMULUS-748 #422

Merged
merged 2 commits into from
Jul 5, 2018
Merged

Marc's final feedback on CUMULUS-748 #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c77310d
CUMULUS-748 PR feedback changes
Jul 5, 2018
bb3973b
Fix typo
Jul 5, 2018
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
24 changes: 14 additions & 10 deletions packages/deployment/lib/lambda.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ const yauzl = require('yauzl');

const { Lambda } = require('kes');


/**
* A sub-class of the Kes Lambda class that changes
* how kes handles Lambda function compression and
Expand Down Expand Up @@ -42,18 +41,19 @@ class UpdatedLambda extends Lambda {
*/

async zipLambda(lambda) {
let msg = `Zipping ${lambda.local}`;
// skip if the file with the same hash is zipped
// and is a valid zip file
if (fs.existsSync(lambda.local)) {
if (await fs.pathExists(lambda.local)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use an asynchronous operation instead of hanging the process on the fs.existsSync call.

try {
await (util.promisify(yauzl.open))(lambda.local); // Verify yauzl can open the .zip file
return Promise.resolve(lambda);
}
catch (e) {
console.log(`${lambda.local} appears to be an invalid zip file, and will be re-built`);
console.log(`${lambda.local} is invalid and will be rebuilt`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor change to the message that's logged.

}
}

let msg = `Zipping ${lambda.local}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this variable declaration down closer to where it's actually used.

const fileList = [lambda.source];
if (lambda.useMessageAdapter) {
const kesFolder = path.join(this.config.kesFolder, 'build', 'adapter');
Expand All @@ -62,12 +62,16 @@ class UpdatedLambda extends Lambda {
}

console.log(`${msg} for ${lambda.name}`);
return utils.zip(lambda.local, fileList)
.then(() => lambda)
.catch((e) => {
console.log(`Error zipping ${e}`);
throw (e);
});

try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is an async function, we can use a try/catch to make it a little more explicit what is going on.

await utils.zip(lambda.local, fileList);
}
catch (e) {
console.log(`Error zipping ${e}`);
throw e;
}

return lambda;
}

/**
Expand Down
190 changes: 89 additions & 101 deletions packages/deployment/test/test_lambda.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,104 +88,92 @@ test.serial('zipLambda: works for lambda using message adapter', async (t) => {
);
});

test.serial(
`zipLambda: given an invalid zip file generated from a previous run,
a new valid lambda file is generated`,
async (t) => {
t.context.lambda.useMessageAdapter = true;
const lambdaLocalOrigin = t.context.lambda.local;
const lambdaRemoteOrigin = t.context.lambda.remote;

// put an empty lambda zip file there as the result of the previous run
const existingLambdaLocal = path.join(
path.dirname(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
);

await fs.writeFile(existingLambdaLocal, 'hello');
t.is(fs.statSync(existingLambdaLocal).size, 5);

const testLambda = new Lambda(t.context.config);
await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
t.truthy(fs.statSync(t.context.lambda.local));
t.true(fs.statSync(t.context.lambda.local).size > 5);
t.is(t.context.lambda.local, existingLambdaLocal);

t.is(
path.basename(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaLocalOrigin)}`
);
t.is(
path.basename(t.context.lambda.remote),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaRemoteOrigin)}`
);
}
);


test.serial(
`zipLambda: for lambda using message adapter, no new file is generated
if the task and message adapter are not updated`,
async (t) => {
t.context.lambda.useMessageAdapter = true;

// put a lambda zip file there as the result of the previous run
const existingLambdaLocal = path.join(
path.dirname(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
);

const existingLambdaRemote = path.join(
path.dirname(t.context.lambda.remote),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.remote)}`
);

await fs.copy(zipFixturePath, existingLambdaLocal);
t.is(fs.statSync(existingLambdaLocal).size, zipFixtureSize);

const testLambda = new Lambda(t.context.config);
await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
t.truthy(fs.statSync(t.context.lambda.local));
t.is(fs.statSync(t.context.lambda.local).size, zipFixtureSize);
t.is(t.context.lambda.local, existingLambdaLocal);
t.is(t.context.lambda.remote, existingLambdaRemote);
}
);

test.serial(
`zipLambda: for lambda using message adapter, a new file is created
if the message adapter is updated`,
async (t) => {
t.context.lambda.useMessageAdapter = true;
const lambdaLocalOrigin = t.context.lambda.local;
const lambdaRemoteOrigin = t.context.lambda.remote;

// put an empty lambda zip file there as the result of the previous run
const existingLambdaLocal = path.join(
path.dirname(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
);

await fs.writeFile(existingLambdaLocal, 'hello');
t.is(fs.statSync(existingLambdaLocal).size, 5);

// message adapter is updated, a new lambda zip file is generated
const adapterHashOrigin = Lambda.messageAdapterZipFileHash;
Lambda.messageAdapterZipFileHash = `${adapterHashOrigin}123`;

const testLambda = new Lambda(t.context.config);
await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
t.truthy(fs.statSync(t.context.lambda.local));
t.true(fs.statSync(t.context.lambda.local).size > 5);
t.not(t.context.lambda.local, existingLambdaLocal);

t.is(
path.basename(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaLocalOrigin)}`
);
t.is(
path.basename(t.context.lambda.remote),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaRemoteOrigin)}`
);
}
);
test.serial('zipLambda: given an invalid zip file generated from a previous run, a new valid lambda file is generated', async (t) => { // eslint-disable-line max-len
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using single quotes for the string, since no string interpolation is being performed. Added // eslint-disable-line max-len to prevent complaints about the long line.

Making this change also changed the indentation of the function, so that's why it looks like there's more changed than there actually is.

t.context.lambda.useMessageAdapter = true;
const lambdaLocalOrigin = t.context.lambda.local;
const lambdaRemoteOrigin = t.context.lambda.remote;

// put an empty lambda zip file there as the result of the previous run
const existingLambdaLocal = path.join(
path.dirname(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
);

await fs.writeFile(existingLambdaLocal, 'hello');
t.is(fs.statSync(existingLambdaLocal).size, 5);

const testLambda = new Lambda(t.context.config);
await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
t.truthy(fs.statSync(t.context.lambda.local));
t.true(fs.statSync(t.context.lambda.local).size > 5);
t.is(t.context.lambda.local, existingLambdaLocal);

t.is(
path.basename(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaLocalOrigin)}`
);
t.is(
path.basename(t.context.lambda.remote),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaRemoteOrigin)}`
);
});


test.serial('zipLambda: for lambda using message adapter, no new file is generated if the task and message adapter are not updated', async (t) => { // eslint-disable-line max-len
t.context.lambda.useMessageAdapter = true;

// put a lambda zip file there as the result of the previous run
const existingLambdaLocal = path.join(
path.dirname(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
);

const existingLambdaRemote = path.join(
path.dirname(t.context.lambda.remote),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.remote)}`
);

await fs.copy(zipFixturePath, existingLambdaLocal);
t.is(fs.statSync(existingLambdaLocal).size, zipFixtureSize);

const testLambda = new Lambda(t.context.config);
await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
t.truthy(fs.statSync(t.context.lambda.local));
t.is(fs.statSync(t.context.lambda.local).size, zipFixtureSize);
t.is(t.context.lambda.local, existingLambdaLocal);
t.is(t.context.lambda.remote, existingLambdaRemote);
});

test.serial('zipLambda: for lambda using message adapter, a new file is created if the message adapter is updated', async (t) => { // eslint-disable-line max-len
t.context.lambda.useMessageAdapter = true;
const lambdaLocalOrigin = t.context.lambda.local;
const lambdaRemoteOrigin = t.context.lambda.remote;

// put an empty lambda zip file there as the result of the previous run
const existingLambdaLocal = path.join(
path.dirname(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(t.context.lambda.local)}`
);

await fs.writeFile(existingLambdaLocal, 'hello');
t.is(fs.statSync(existingLambdaLocal).size, 5);

// message adapter is updated, a new lambda zip file is generated
const adapterHashOrigin = Lambda.messageAdapterZipFileHash;
Lambda.messageAdapterZipFileHash = `${adapterHashOrigin}123`;

const testLambda = new Lambda(t.context.config);
await testLambda.zipLambda(testLambda.buildS3Path(t.context.lambda));
t.truthy(fs.statSync(t.context.lambda.local));
t.true(fs.statSync(t.context.lambda.local).size > 5);
t.not(t.context.lambda.local, existingLambdaLocal);

t.is(
path.basename(t.context.lambda.local),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaLocalOrigin)}`
);
t.is(
path.basename(t.context.lambda.remote),
`${Lambda.messageAdapterZipFileHash}-${path.basename(lambdaRemoteOrigin)}`
);
});