-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add 'package' command and SAM support #258
Conversation
This reverts commit 87bd6af.
I'm proposing making a breaking change and removing authorizer_id in favor of authorizer_name. The rationale for this is two-fold. One, as far as I can tell, it's impossible to support the old way of using authorizer_id. This is because the swagger document defines the entire API, and it appears to (at least conceptually) wipe out the old stuff before putting in the new. Therefore it's not possible to refer to a pre-existing authorizer by id, it has to be called out in the document. The second reason is that I think this is the right long term fix. I want the chalice app to define all the relevant parts of the api in code, and being able to define the authorizers in your app.py and allow chalice to create it for you seems ideal. Before this change, the workflow was basically 1) Create the authorizer either via the CLI, console, or API. 2) Find it's auto generated ID. 3) Specify that authorizer_id in an @app.route(...) call. Now the workflow is you specify the authorizer definition in your app file, and your @app.route(...) references that authorizer by name.
Codecov Report
@@ Coverage Diff @@
## package #258 +/- ##
===========================================
+ Coverage 79.27% 82.91% +3.63%
===========================================
Files 13 17 +4
Lines 1515 1539 +24
Branches 200 193 -7
===========================================
+ Hits 1201 1276 +75
+ Misses 257 212 -45
+ Partials 57 51 -6
Continue to review full report at Codecov.
|
chalice/deploy/packager.py
Outdated
# args, so we need to patch out sys.argv with the venv | ||
# dir. The original sys.argv is replaced on exit. | ||
original = sys.argv | ||
sys.argv = ['', venv_dir, '--quiet'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use virtualenv. create_environment()
instead of monkeypatching sys.argv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I wasn't aware of create_environment()
, that's certainly preferable to patching out sys.argv. I'll give this a shot.
os.makedirs(dirname) | ||
with zipfile.ZipFile(package_filename, 'w', | ||
compression=zipfile.ZIP_DEFLATED) as z: | ||
self._add_py_deps(z, deps_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to fail for any C package unless the host OS is on a compatible with Lambda Linux right? Would it be better to instead do something like:
$ pip wheel -r requirements.txt
$ # Delete any wheel files that are not pure Python, recording the names of the files deleted.
$ pip download --only-binary=:all: --platform manylinux1_x86_64 --no-deps <the list of names from above>
$ # Fail if there are any downloads that didn't get satisified
Once you have a list of wheels, it's not particularly hard to "install" them manually into a zip file for unpacking later.
The advantage of this is that it will generally allow binary packages (where possible), but will fail loud and obviously when those binary packages do not exist. You could possibly do CC=/bin/false
when running pip wheel
which will force packages with optional C extensions to fail at building (typically), but not all of them will actually produce a pure python wheel (e.g. PyYAML produced a wheel that internally is pure python, but whose filename still marks it as binary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would definitely be better. That work is tracking in #249.
I should have clarified in the diff, but this packager.py
code is all pre-existing, but is showing up as new code because I restructured some of the files. I definitely want to circle back on this, a lot of this is old code that could use some rework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha. That makes sense then.
This adds support for a 'package' command. This command takes the current application and produces artifacts that can subsequently be deployed by cloudformation. The intent is to allow for decoupling the packaging from the deployment process, which provides a number of benefits. First, something besides chalice can do the deployment. For many customers that have existing deployment tooling based around cloudformation, they would be able to leverage these tools to handle deployment. In addition to existing customer tooling, this also makes it possible to integrate with other AWS services. By creating a separate app package, chalice can now integrate with AWS CodePipeline. This allows a user to set up a full CD pipeline that deploys your chalice app whenver you 'git push' to a repo. The second benefit is that by decoupling the packaging from the deployment, this allows a user to modify the app package before deploying. While chalice strives to expose APIs that make app configuration simple, the user can now have a system in place that packages the app, modifies the app package, and then deploys the app. Missing pieces: * Updating the 'deploy' command to be able to deploy an app package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
This adds support for a 'package' command. This command takes the
current application and produces artifacts that can subsequently be
deployed by cloudformation. The intent is to allow for decoupling the
packaging from the deployment process, which provides a number of
benefits. First, something besides chalice can do the deployment. For
many customers that have existing deployment tooling based around
cloudformation, they would be able to leverage these tools to handle
deployment. In addition to existing customer tooling, this also makes
it possible to integrate with other AWS services. By creating a
separate app package, chalice can now integrate with AWS CodePipeline.
This allows a user to set up a full CD pipeline that deploys your
chalice app whenver you 'git push' to a repo.
The second benefit is that by decoupling the packaging from the
deployment, this allows a user to modify the app package before
deploying. While chalice strives to expose APIs that make app
configuration simple, the user can now have a system in place that
packages the app, modifies the app package, and then deploys the app.
Missing pieces:
NOTE: This is branched off of
add-authorizers
so it includes that PR as well (#256)