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.

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

hooks: lambda: allow uploading pre-built payloads #564

Merged
merged 3 commits into from
May 13, 2019

Conversation

danielkza
Copy link
Contributor

No description provided.

Copy link
Contributor

@ejholmes ejholmes left a comment

Choose a reason for hiding this comment

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

Few minor comments, but otherwise this looks useful!

includes,
excludes,
follow_symlinks)
return _upload_prebuilt_zip(s3_conn, bucket, prefix, name, options,
Copy link
Contributor

Choose a reason for hiding this comment

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

Kinda bugs me that we have to methods that call _upload_code. I think _upload_prebuilt_zip and _build_and_upload_zip should be changed so that we return a tuple (zip_file, version) and then we call _upload_code once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've kept them separated due to the prebuilt case passing the open file through instead of loading all the contents. When that happens _upload_code needs to be called in the scope of the context manager, which will require some weird conditionals if the call is unified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. _build_and_upload_zip currently doesn't use the context manager when passing the zip file down to _upload_code. Think we should change it so we're consistent?

@@ -93,6 +93,18 @@ def _calculate_hash(files, root):
return file_hash.hexdigest()


def _calculate_prebuilt_hash(f):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just re-use _calculate_hash.

_calculate_hash("path/to/zip-file.zip")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That won't be the exact MD5 of the file, as it will append a null character to the stream when calculating.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a problem? We just need a consistent hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, but it seemed more natural to me (as I just used md5sum to figure out the reference values).

@danielkza danielkza force-pushed the lambda-hook-prebuilt-payload branch from 1323b63 to 6824154 Compare July 3, 2018 21:26
@phobologic
Copy link
Member

@ejholmes @danielkza - thinking about making a 1.4 release soon. Should this be in it? What's left to figure out?

@danielkza danielkza force-pushed the lambda-hook-prebuilt-payload branch from 6824154 to 1917033 Compare July 8, 2018 07:49
@danielkza
Copy link
Contributor Author

@phobologic Rebased and refactored a bit to reduce code duplication.

@danielkza danielkza force-pushed the lambda-hook-prebuilt-payload branch from 1917033 to e359e10 Compare March 29, 2019 19:13
@danielkza danielkza force-pushed the lambda-hook-prebuilt-payload branch from e359e10 to 359619a Compare March 29, 2019 19:17
@danielkza
Copy link
Contributor Author

Rebased, including a refactor of the Lambda hook tests.

@danielkza
Copy link
Contributor Author

Fixed the broken tests.

Copy link
Contributor

@ejholmes ejholmes left a comment

Choose a reason for hiding this comment

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

lgtm!

@phobologic phobologic merged commit fde9bd4 into cloudtools:master May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants