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

Improve Lambda packaging #986

Merged
merged 3 commits into from Feb 28, 2017
Merged

Improve Lambda packaging #986

merged 3 commits into from Feb 28, 2017

Conversation

chadwhitacre
Copy link
Contributor

Proposed fix for #193. Rather than guessing where library dependencies reside, just sniff an actual dependency. Hopefully this provides the greatest portability?

@chadwhitacre
Copy link
Contributor Author

What do you think of this approach, @kapilt?

@chadwhitacre
Copy link
Contributor Author

Blorg. Drone why u no like?

Marking wip ... 😞

@chadwhitacre chadwhitacre changed the title Improve Lambda packaging [wip] Improve Lambda packaging Feb 25, 2017
@kapilt
Copy link
Collaborator

kapilt commented Feb 26, 2017

i sanity checked the server (was curious about memory error raised), but it does appear to be specific to this pr.

@chadwhitacre
Copy link
Contributor Author

Rebased, was 35e03a769e6bb876c37c5d0bd8e1152cca99c8e5.

@chadwhitacre
Copy link
Contributor Author

Ready for review, @kapilt ...

@chadwhitacre chadwhitacre changed the title [wip] Improve Lambda packaging Improve Lambda packaging Feb 27, 2017
c7n/mu.py Outdated
f_path = os.path.join(root, f)
dest_path = os.path.join(arc_prefix, f)
self.add_file(f_path, dest_path)
venv_lib_path = os.path.dirname(ipaddress.__file__)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should not be hacked into the middle of an generic function, let's try to localize this to custodian_archive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 691592f.

Rather than guessing where library dependencies reside, just sniff an
actual dependency. Hopefully this provides the greatest portability?
This test manually creates a `PythonPackageArchive`, and thus it
benefiteth not from the "aggressive shrinking" of `custodian_archive`.
Here we can be even more aggressive.
We want to keep mu decoupled from custodian as much as we can. This
commit replaces the virtualenv_dir parameter of PythonPackageArchive
with lib_paths, since the former was only used for computing
venv_lib_paths anyway. Effectively then we are pushing the
responsibility for computing lib_paths up to the caller.
Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@kapilt kapilt merged commit 029f6dd into cloud-custodian:master Feb 28, 2017
@chadwhitacre chadwhitacre deleted the improve-lambda-packaging branch February 28, 2017 16:02
@kapilt
Copy link
Collaborator

kapilt commented Mar 2, 2017

looks like this ended up breaking other library users of mu including mailer.

kapilt pushed a commit to kapilt/cloud-custodian that referenced this pull request Mar 2, 2017
kapilt added a commit that referenced this pull request Mar 2, 2017
* Revert "mu.py handle system python installation when lambda packaging (#986)"

This reverts commit 029f6dd.

* micro increment version
@chadwhitacre
Copy link
Contributor Author

chadwhitacre commented Mar 2, 2017

😞

@chadwhitacre chadwhitacre restored the improve-lambda-packaging branch March 2, 2017 09:26
This was referenced Mar 2, 2017
@chadwhitacre chadwhitacre added this to Done in blue team Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants