-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add full custom resource merging support #1844
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
Conversation
Key: Value | ||
Outputs: | ||
CustomOutput: | ||
Value: "My Custom Output" |
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.
Would add a Description here as well. Its optional, but if people see it in the docs they know its available
@pmuens could you also add an example here as a comment with a service that overwrites a resource that we create to validate this. So not just overwriting like with an output or Description, but merging in additional config items |
@flomotlik here's an example which merges in a new description for the
We're not able to overwrite CloudFormation resources which are created with the I've also added a test which overwrites the serverless/lib/plugins/aws/deploy/tests/createStack.js Lines 186 to 205 in 121df4d
|
Then we should change that. We definitely have to support overwriting anything we create through custom resources. Otherwise people simply won't be able to do it at all. The only real solution I see is merging it back in during before:deploy:deploy. |
This section is used to store the core CloudFormation template and merge compiled resources into it.
@flomotlik just pushed the refactoring. Now all the stuff is merged into the Overwriting the whole template is as easy as using simply the Here's an example service file which shows how this can be done. A I've added all available events so that we validate that everything works (which does on my machine and the integration tests).
|
this.serverless.service.resources.Outputs = | ||
this.serverless.service.resources.Outputs || {}; | ||
_.merge(this.serverless.service.resources.Outputs, newOutputEndpointObject); | ||
this.serverless.service.provider.compiledCloudFormationTemplate.Outputs = |
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.
Don't think this line is necessary any more as the core template will create Outputs already
Because it's already available at the beginning of the deploy plugin as the core CloudFormation template will be attached there (which is then used to merge other resources into it later on).
I tested it with all the above mentioned Events (http, S3, SNS, Schedule) and it worked with all of them. One thing that does fail though if the
Error Message:
Looks like its simply replacing the whole Resources section with null in that case. Probably best to set Resources and outputs to empty hashes in case they are null. Or we simply properly document it but this could imho be a bad experience. |
Yep sounds good (I would do both, setting it to empty hashes and document it). |
Looks nice guys!!! Not completely sure about the Some food for thought: functions:
hello:
handler: handler.hello
events:
- hello-get:
type: http
config:
path: hello
method: get
- hello-head:
type: http
config:
path: hello
method: head
- hello-s3-jpg
type: s3
config:
bucket: HelloS3Bucket,
bucketEvents:
- s3:ObjectCreated
filterRules:
- name: suffix
value: jpg
- name: prefix
value: foo/
- hello-s3-bar
type: s3
config:
bucket: HelloS3Bucket,
bucketEvents:
- s3:ObjectCreated
filterRules:
- name: suffix
value: png
- name: prefix
value: bar/
- hello-sns
type: sns
- hello-schedule
type: schedule
config:
schedule: rate(1 minute)
### If you want to overwrite/add extra information
resources:
Resources:
HelloS3Bucket:
Type: AWS::S3::Bucket
HelloS3Jpg:
Type: AWS::S3::Bucket
Properties:
Description: 'Description for S3 event'
HelloSns:
Properties:
DisplayName: 'Description for SNS topic'
# Etc... Or short-hand with functions:
hello:
handler: handler.hello
events:
- http:
path: hello
method: get
- http:
path: hello
method: head
- s3:
logical-id: hello-s3-jpg
bucket: HelloS3Bucket,
bucketEvents:
- s3:ObjectCreated
filterRules:
- name: suffix
value: jpg
- name: prefix
value: foo/
- s3:
logical-id: hello-s3-png
bucket: HelloS3Bucket,
bucketEvents:
- s3:ObjectCreated
filterRules:
- name: suffix
value: png
- name: prefix
value: bar/
- sns:
logical-id: hello-sns
- schedule:
logical-id: hello-schedule
schedule: rate(1 minute)
### If you want to overwrite/add extra information
resources:
Resources:
HelloS3Bucket:
Type: AWS::S3::Bucket
HelloS3Jpg:
Type: AWS::S3::Bucket
Properties:
Description: 'Description for S3 event'
HelloSns:
Properties:
DisplayName: 'Description for SNS topic'
# Etc... |
@nicka we have to change the naming of resources. At the moment we have some inconsistencies there, e.g. the SNS topic doesn't have the SNS topic name in the logical name for example. Other parts (like S3) get a number added at the end, .... This needs to be cleaned up before a next release so its consistent and documented. I like the idea of adding logical-id as a potential option to events so it can be set directly (although sometimes one event could mean multiple CF resources, so one name might not be enough) |
@pmuens could you put the fix for the resources in place, then I think this is ready to be merged |
@flomotlik done |
e20881b
to
6b3d59e
Compare
Updated the last commit with a quick fix should be ready now. |
🚀 |
This PR implements the full support for custom resource merging as discussed in #1833
/cc @flomotlik @eahefnawy