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

get rid of duplicates and extraneous libraries #581

Merged
merged 4 commits into from
Jan 7, 2017

Conversation

kapilt
Copy link
Contributor

@kapilt kapilt commented Jan 4, 2017

they are either already present in the python lambda environment or extraneous there (dist-info).

For a simple bottle app, this shrinks the compressed zip from 9.3mb to 2.3mb.

first part from #556, lots more low hanging fruit.

@kapilt
Copy link
Contributor Author

kapilt commented Jan 4, 2017

travis ci setup seems to have been broken for a while... also getting errors running tests locally (slow) and error when AWS_DEFAULT_REGION is set and does not equal eu-west-1 ...

@Miserlou
Copy link
Owner

Miserlou commented Jan 4, 2017

Thanks for this PR, sorry for the New Years absence, gonna try to fix the Coveralls/Travis issue before I tackle the PR backlog - #582

@Miserlou
Copy link
Owner

Miserlou commented Jan 4, 2017

Okay, builds fixed. Stupid Coverage.

It looks like your solution should actually just be added in here: https://github.com/Miserlou/Zappa/blob/master/zappa/zappa.py#L197 rather than in the CLI. Make sense?

Happy new year!

@coveralls
Copy link

coveralls commented Jan 4, 2017

Coverage Status

Changes Unknown when pulling 03ec1b0 on kapilt:exclude-duplicate-modules into ** on Miserlou:master**.

@kapilt
Copy link
Contributor Author

kapilt commented Jan 4, 2017

i was thinking about that, the issue is that if a user explicitly wants a more recent version of boto then in the lambda env, preserving it a a default config option for the user gives them the choice, but happy to move it over to be always on thing.

@Miserlou
Copy link
Owner

Miserlou commented Jan 4, 2017

Our general philosophy is be practical and sane by default, and for anything else require the user to be completely explicit.

@Miserlou
Copy link
Owner

Miserlou commented Jan 4, 2017

With the notable caveat that such abilities should be adequately documented. :D

@kapilt
Copy link
Contributor Author

kapilt commented Jan 4, 2017

right but isn't what was done practical and sane by default, with the option that gives the user the choice to override, by just making it the default value for the exclude setting?. putting it in zappa.py ZIP_EXCLUDES removes the choice since then it becomes a non overridable hard-coded list. anyways happy to do it either way, your call.

@Miserlou
Copy link
Owner

Miserlou commented Jan 5, 2017

[From a discussion on Slack, it appears that ZIP_EXCLUDES isn't replaced by the supplied excludes, but actually it's appended.]

@kapilt
Copy link
Contributor Author

kapilt commented Jan 5, 2017

so to recap, the default exclude setting, which can be user overridden on a stage basis, will now default to packages already present in the lambda environment. the user can override this value if they want to include a newer version then what is present in the lambda env, and the default is documented in the readme. dist-info was included into the permanent excludes in zappa.py ZIP_EXCLUDES as there is no value for user override of it.

@coveralls
Copy link

coveralls commented Jan 5, 2017

Coverage Status

Changes Unknown when pulling 1114b39 on kapilt:exclude-duplicate-modules into ** on Miserlou:master**.

@coveralls
Copy link

coveralls commented Jan 5, 2017

Coverage Status

Changes Unknown when pulling b16a03d on kapilt:exclude-duplicate-modules into ** on Miserlou:master**.

@coveralls
Copy link

coveralls commented Jan 5, 2017

Coverage Status

Changes Unknown when pulling 0c03346 on kapilt:exclude-duplicate-modules into ** on Miserlou:master**.

@Miserlou
Copy link
Owner

Miserlou commented Jan 7, 2017

I'm going to merge this in now because you've been really patient, but I'm going to manually take out the dist-info because it turns out we actually need that for any resources which use pkg_resources, ex 6c90d2e

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