Update package command to work with new deployer #772
Conversation
This moves the loading of the IAM policy specified in a FileBasedIAMPolicy() from the planning stage to the build stage. This allows consumers of the FileBasedIAMPolicy to not have to duplicate code and load the IAM policy from disk. This allows code reuse between the deployer and the packager.
This brings numerous benefits: 1. Full parity with `chalice deploy`. This means you can use the package command for pure lambda functions, built in authorizers, and scheduled events. 2. We'll now get exceptions raised when a new resource isn't supported in the `package` command instead of silently ignoring the resource like we do now. The generated template is slightly different from the old deployer, notably: 1. The `Events` key is no longer used for `Api` types. Instead we just manually create a lambda permission resource that allows api gateway to invoke the API handler. This also more closely matches what `chalice deploy` does. 2. The `Policies` is just the generated policy (when it applies). Previously the value was a list of policies, but chalice only generates a single policy.
|
Nice I played around with this quite a bit and it seems to work well. |
There was a problem hiding this comment.
Looks good. I tested a bunch of this out as well and looked fine. I just had some comments/questions. Not sure if we should do this now, but it may be cool if we can reuse the integration tests to test that all of the cloudformation logic has feature parity with chalice deploy since we are now enforcing it.
chalice/package.py
Outdated
| continue | ||
| original_location = resource['Properties']['CodeUri'] | ||
| new_location = os.path.join(outdir, 'deployment.zip') | ||
| self._osutils.copy(original_location, new_location) |
There was a problem hiding this comment.
Are we copying over the deployment.zip for every lambda/SAM resource? Is it possible to make sure we do this once only if we are?
There was a problem hiding this comment.
Yeah we are. I've updated this.
| assert properties['DefinitionBody'] == {'swagger': 'document'} | ||
|
|
||
|
|
||
| def test_can_inject_environment_vars(sample_app, |
There was a problem hiding this comment.
Looks like we missed this one when porting over the tests. I also cannot find it in the ported over implementation.
|
|
||
| def _default(self, resource, template): | ||
| # type: (models.Model, Dict[str, Any]) -> None | ||
| raise NotImplementedError(resource) |
There was a problem hiding this comment.
I wonder if it makes sense to add a test for this? It looks like it would work but may be nice to double check.
| 'Handler': resource.handler, | ||
| 'CodeUri': resource.deployment_package.filename, | ||
| 'Tags': resource.tags, | ||
| 'Timeout': resource.timeout, |
There was a problem hiding this comment.
Some of these values used to have if is not None check on it. It may be good to check that these values are actually never None to make sure it does not affect the generated cloudformation template.
There was a problem hiding this comment.
This happens at a separate layer above. We previously had to check at this layer because we were using the config object directly. I'll double check, but I'm pretty sure it's now impossible for these values to be None now that we're using the models.
|
@kyleknap I believe I've fixed everything. Let me know if I missed anything. |
This brings numerous benefits:
chalice deploy. This means you can use thepackage command for pure lambda functions, built in authorizers,
and scheduled events.
in the
packagecommand instead of silently ignoring the resourcelike we do now.
The generated template is slightly different from the old deployer,
notably:
Eventskey is no longer used forApitypes. Instead we justmanually create a lambda permission resource that allows api gateway to
invoke the API handler. This also more closely matches what
chalice deploydoes.Policiesis just the generated policy (when it applies).Previously the value was a list of policies, but chalice only generates
a single policy.
Here's an worked example with diffs of the old vs. new packager: https://gist.github.com/jamesls/c80880fb7df39bc33ef4d30aca7d6995