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 argument to deep-merge in a JSON file during packaging #1195

Closed
wants to merge 2 commits into from

Conversation

stealthycoin
Copy link
Contributor

Its a common use case to need to modify a Chalice generated template
before deployment. Often to inject resources, values, or
configurations that are not supported directly by Chalice. It will
always be the case that something on AWS is not supported by Chalice
directly that a consumer may want to interact with. This PR opens up
an interface to address that gap more easily in Chalice itself.

The package command can now be invoked with a new argument:

$ chalice package --merge-template extras.json out

This extras.json file should be a JSON formatted file which will be
deep-merged on top of the sam.json that is generated by Chalice.

For a simple example lets assume that we have the default new Chalice
project and that extras.json has the following content:

{
    "Resources": {
	"APIHandler": {
	    "Properties": {
		"Environment": {
		    "Variables": {
			"foo": "bar"
		    }
		}
	    }
	}
    }
}

The generated template located at out/sam.json will have the this
environment variable injected into it:

...
...
    "APIHandler": {
      "Type": "AWS::Serverless::Function",
      "Properties": {
        "Runtime": "python3.6",
        "Handler": "app.app",
        "CodeUri": "./deployment.zip",
        "Tags": {
          "aws-chalice": "version=1.9.1:stage=dev:app=test"
        },
        "Timeout": 60,
        "MemorySize": 128,
        "Role": {
          "Fn::GetAtt": [
            "DefaultRole",
            "Arn"
          ]
        },
        "Environment": {
          "Variables": {
            "foo": "bar"
          }
        }
      }
    },
...
...

Obviously this simple example could have been achieved using the
config file for the environment variables. But using the extras.json
file we could have injected a DynamoDB table into the template, and
injected the table name as a REF into the APIHandler's environment
variables.

For the time being the merge functionality is relatively simple. It can
be extended over time as we discover what types of merge extensions
are needed.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Its a common use case to need to modify a Chalice generated template
before deployment. Often to inject resources, values, or
configurations that are not supported directly by Chalice. It will
always be the case that something on AWS is not supported by Chalice
directly that a consumer may want to interact with. This PR opens up
an interface to address that gap more easily in Chalice itself.

The package command can now be invoked with a new argument:

  $ chalice package --merge-template extras.json out

This extras.json file should be a JSON formatted file which will be
deep-merged on top of the sam.json that is generated by Chalice.

For a simple example lets assume that we have the default new Chalice
project and that extras.json has the following content:

```
{
    "Resources": {
	"APIHandler": {
	    "Properties": {
		"Environment": {
		    "Variables": {
			"foo": "bar"
		    }
		}
	    }
	}
    }
}
```

The generated template located at out/sam.json will have the this
environment variable injected into it:

```
...
...
    "APIHandler": {
      "Type": "AWS::Serverless::Function",
      "Properties": {
        "Runtime": "python3.6",
        "Handler": "app.app",
        "CodeUri": "./deployment.zip",
        "Tags": {
          "aws-chalice": "version=1.9.1:stage=dev:app=test"
        },
        "Timeout": 60,
        "MemorySize": 128,
        "Role": {
          "Fn::GetAtt": [
            "DefaultRole",
            "Arn"
          ]
        },
        "Environment": {
          "Variables": {
            "foo": "bar"
          }
        }
      }
    },
...
...
```

Obviously this simple example could have been achieved using the
config file for the environment variables. But using the extras.json
file we could have injected a DynamoDB table into the template, and
injected the table name as a REF into the APIHandler's environment
variables.

For the time being the merge functionality is relatively simple. It can
be extended over time as we discover what types of merge extensions
are needed.
@codecov-io
Copy link

codecov-io commented Jul 30, 2019

Codecov Report

Merging #1195 into master will increase coverage by <.01%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
+ Coverage   95.85%   95.85%   +<.01%     
==========================================
  Files          28       28              
  Lines        5021     5071      +50     
  Branches      636      642       +6     
==========================================
+ Hits         4813     4861      +48     
- Misses        135      136       +1     
- Partials       73       74       +1
Impacted Files Coverage Δ
chalice/cli/__init__.py 86.98% <100%> (+0.04%) ⬆️
chalice/package.py 100% <100%> (ø) ⬆️
chalice/cli/factory.py 91.25% <50%> (-1.16%) ⬇️

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 bd7fa76...d49d9c5. Read the comment docs.

@kapilt
Copy link
Contributor

kapilt commented Jul 30, 2019

fwiw, I like the idea, it came up in discussion last week with @jamesls and @kyleknap, partly in reference to the terraform pr which also supports forward/backward refs. the main usage caveat is that most folks are using yaml these days, for which it would be nice to support directly (at least on the read side). I'll also note that using these refs also tends to enshrine that apigw is backed by a single lambda function (apihandler) which may not be always be the case, but the general thought is that the template constructs become part of the chalice public interface if folks start depending on them via reference in their automation templates.


def _merge_replace(self, src, dst):
# type: (Any, Any) -> None
# Default merge behavior is to take the source value. This behaivor is
Copy link
Contributor

Choose a reason for hiding this comment

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

speling typo

@kyleknap kyleknap mentioned this pull request Aug 1, 2019
Copy link
Member

@jamesls jamesls left a comment

Choose a reason for hiding this comment

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

In general, I think this would be a great feature so I'd love to get this merged.

I think this does make the cfn template we generate much more frozen in terms of the resources we create and the names we use for our existing abstractions. This was already sorta-kinda the case, but I think that this would definitely make it much more strict.

Perhaps as a rule of thumb going forward, the cfn template we generate via chalice package will always use the same names/resources unless you opt-in to new behavior. So to @kapilt's point, if we do support per-lambda API gateway routes, it would have to be opt-in (within this major version) via config or something so we don't break customers that may be using this merging functionality.

chalice/config.py Outdated Show resolved Hide resolved
chalice/package.py Outdated Show resolved Hide resolved
chalice/package.py Outdated Show resolved Hide resolved
tests/unit/test_package.py Outdated Show resolved Hide resolved
chalice/package.py Outdated Show resolved Hide resolved
chalice/package.py Outdated Show resolved Hide resolved
chalice/package.py Outdated Show resolved Hide resolved
* Simplify merger implementation
* Pull multiple post processor handling out of the AppPackager and into
  a composite post processor
* Make DeepMerger injectable into the TemplateMergePostProcessor to
  allow different mergers to be injected in the future
* Remove config file option, instead you must specify the command line
  option
* Add changelog entry
* Add documentation section for merging under the cfn topic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants