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

Add 'package' command and SAM support #258

Merged
merged 3 commits into from
Mar 24, 2017
Merged

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Mar 18, 2017

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.

NOTE: This is branched off of add-authorizers so it includes that PR as well (#256)

jamesls added 2 commits March 14, 2017 21:09
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-io
Copy link

codecov-io commented Mar 18, 2017

Codecov Report

Merging #258 into package will increase coverage by 3.63%.
The diff coverage is 84.58%.

@@             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
Impacted Files Coverage Δ
chalice/awsclient.py 81.56% <100%> (+0.03%) ⬆️
chalice/app.py 95.14% <100%> (+0.07%) ⬆️
chalice/deploy/deployer.py 71.42% <71.42%> (ø)
chalice/utils.py 79.16% <79.16%> (ø)
chalice/cli/__init__.py 55.95% <80%> (+0.05%) ⬆️
chalice/deploy/packager.py 90.75% <90.75%> (ø)
chalice/package.py 97.1% <97.1%> (ø)
chalice/deploy/swagger.py 98.41% <98.41%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac99183...8af7d20. Read the comment docs.

# 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']
Copy link

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?

Copy link
Member Author

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)
Copy link

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).

Copy link
Member Author

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.

Copy link

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.
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

🚢

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.

4 participants