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

[Feature] Adding support for passing local paths to nested CloudFormation stacks #2344

Closed
wants to merge 2 commits into from
Closed

[Feature] Adding support for passing local paths to nested CloudFormation stacks #2344

wants to merge 2 commits into from

Conversation

dtimberlake
Copy link
Contributor

This is a feature I've been using with aws cloudformation package for my own work and thought I'd rework the code and add tests for your consideration for a merge.

The idea is to map references in nested stacks to parameters passed in from it's parent stack. The reference is parsed and replaced with the parameter's value (if the value is not dependent on a deploy time parameter) before the package command exports the resource's artifact to S3.


Example:

# package.yaml
AWSTemplateFormatVersion: 2010-09-09
Resources:
  ApiStack:
    Type: AWS::CloudFormation::Stack
    Properties:
      TemplateURL: ./api-template.yaml
      Parameters:
        SwaggerDoc: /path/to/swagger.yaml


# api-template.yaml
AWSTemplateFormatVersion: 2010-09-09
Parameters:
  SwaggerDoc:
    Type: String
Resources:
  MyFunction:
    Type: AWS::ApiGateway::RestApi
    Properties:
        BodyS3Location: !Ref SwaggerDoc

The command maps the parameter SwaggerDoc in api-template.yaml to the parameter value passed in from package.yaml (/path/to/swagger.yaml) before iterating over the resources. When it comes to the BodyS3Location property of MyFunction it will replace the !Ref SwaggerDoc with the value /path/to/swagger.yaml, then export that file to S3.


This feature also converts any relative paths to absolute paths by merging with the calling template's parent directory's path.


Example:

./swagger.yaml would become /absolute/path/to/swagger.yaml


This only affects the the specific properties of resources specified here. Any properties not specified in the previous link are not affected.

I find this really helps keep things DRY when composing multiple connected local templates.

I'd love y'all's feedback.

@JordonPhillips
Copy link
Member

Thanks for the pr! I'll have a look.

Looping in the service team:
cc @sanathkr

Copy link
Member

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

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

The implementation mostly seems good, though I think a service team member needs to look at this sort of change.

@@ -206,14 +206,18 @@ class Resource(object):

PROPERTY_NAME = None

def __init__(self, uploader):
def __init__(self, uploader, template_params={}):
Copy link
Member

Choose a reason for hiding this comment

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

Mutable objects should not be used for default values.

@@ -364,7 +407,7 @@ class Template(object):
"""

def __init__(self, template_path, parent_dir, uploader,
resources_to_export=EXPORT_DICT):
resources_to_export=EXPORT_DICT, template_params={}):
Copy link
Member

Choose a reason for hiding this comment

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

Mutable default value

test_template_params = {
"Parameter1": "./relative/filepath.txt"
}
stack_resource = CloudFormationStackResource(self.s3_uploader_mock, template_params=test_template_params)
Copy link
Member

Choose a reason for hiding this comment

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

I know we've already got lots of code in here that breaks this rule, but please stick to < 80 characters per line.

@dtimberlake
Copy link
Contributor Author

@JordonPhillips Changes mentioned have been made

@nikolay
Copy link

nikolay commented Apr 11, 2017

@JordonPhillips Any update?

@KaibaLopez KaibaLopez removed the large label Feb 26, 2020
@kdaily kdaily added the customization Issues related to CLI customizations (located in /awscli/customizations) label Aug 26, 2021
@justindho justindho linked an issue May 17, 2022 that may be closed by this pull request
2 tasks
@justindho
Copy link
Contributor

Hi @dtimberlake,

Our team just put out a recent proposal in #6828 detailing improvements to the contribution process. We are working through open PRs and are trying to determine where this issue falls.

For feature requests, to invest the time in reviewing it, we would like to make sure the feature has wider community interest and are looking for 10 👍 votes on the issue that I've created for this open PR. For the currently open PR, we are going to set it as a draft. Once the 👍 threshold is met, we will move the issue to the “Implementation” state and re-review the implementation plan in this PR.

@stevehaneytrailblazer
Copy link

With the recent change on cfn-lint v0.66.0 (aws-cloudformation/cfn-lint#2382) relative path Urls like the one above (when using package command) are now returning warnings that cause Git Action failures. If this could be added to the CLI as well that'd be great to make it a non warning.

@tim-finnigan
Copy link
Contributor

HI @dtimberlake thanks for creating this PR and for your patience. Could you please rebase this PR against the latest branch for the team’s further consideration going forward?

@tim-finnigan tim-finnigan added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 5, 2023
@tim-finnigan
Copy link
Contributor

Checking in again - I'm going to close this PR for now as we have not heard back regarding the request to rebase. But we are continuing to track the feature request in #6961.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloudformation package-deploy community customization Issues related to CLI customizations (located in /awscli/customizations) needs-rebase response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support passing local paths to nested CloudFormation stacks
9 participants