Skip to content

Support terraform source_paths as list of dicts - #4506

Merged
mildaniel merged 18 commits into
aws:developfrom
tam116:develop
Jan 10, 2023
Merged

Support terraform source_paths as list of dicts#4506
mildaniel merged 18 commits into
aws:developfrom
tam116:develop

Conversation

@tam116

@tam116 tam116 commented Dec 20, 2022

Copy link
Copy Markdown
Contributor

Which issue(s) does this change fix?

#4495

Why is this change necessary?

Terraform supports specifying source path several different ways including as a list of maps which translate to a list of dicts in SAM's python implementation

How does it address the issue?

Currently sam cli's terraform hook handles strings, list of strings, and dicts, but not list of dicts.

What side effects does this change have?

None

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • [N/A] Add input/output type hints to new functions/methods
  • [N/A] Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • [N/A] Write/update integration tests
  • [N/A] Write/update functional tests if needed
  • make pr passes
  • [N/A] make update-reproducible-reqs if dependencies were changed
  • [N/A] Write documentation

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

@tam116
tam116 requested a review from a team as a code owner December 20, 2022 21:40
@tam116
tam116 requested review from hnnasit and torresxb1 December 20, 2022 21:40
@github-actions github-actions Bot added pr/external stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. labels Dec 20, 2022
@sriram-mv

Copy link
Copy Markdown
Contributor

Thanks for the PR, could you showcase an example and also look at adding integration tests?

@tam116

tam116 commented Dec 22, 2022

Copy link
Copy Markdown
Contributor Author

I think my change is covered by the existing integration tests

There are four ways to specify the source_path when using the 'terraform-aws-modules/lambda/aws' module. Terraform's documentation shows examples for both Style 1 and Style 4

module "my_lambda" {
  source = 'terraform-aws-modules/lambda/aws'
  # other configuration items

  # Style 1
  source_path = "/some/file/path"

  # Style 2
  source_path = ["/some/file/path"]
  
# Style 3
  source_path = {
    path = "/some/file/path"
    prefix_in_zip = "python"
    pip_requirements = true
  }

  # Style 4
  source_path = [
    {
      path = "/some/file/path"
      prefix_in_zip = "python"
      pip_requirements = true
    }
  ]
}

Previously only Styles 1,2, and 3 were supported by sam cli. If Style 4 was used sam cli would try to validate the path by calling isabs({path = "/some/file/path", prefix_in_zip = "python", pip_requirements = true}) which failed with "TypeError: expected str, bytes or os.PathLike object, not dict"

Since the existing _get_source_code_path() function already handles checking if the source_path is a string, dict, or list I thought the best way to support Style 4 would be to modify the "if list" logic and have it call _get_source_code_path() but passing the first element of the list as the source_path

Let me know if you have another questions about this PR

@mildaniel mildaniel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes sense to me, thanks for the contribution! I left a small question.

I also agree with @sriram-mv on the integration testing. Let's add a case that explicitly tests for source_path defined as a list of dicts. Since this wasn't working before, we probably aren't testing this exact case today. We can just add the a new module to one of the existing test cases with this particular format.

Comment thread samcli/hook_packages/terraform/hooks/prepare/enrich.py Outdated
moelasmar and others added 12 commits December 30, 2022 08:12
…4518)

Bumps [setuptools](https://github.com/pypa/setuptools) from 54.2.0 to 65.5.1.
- [Release notes](https://github.com/pypa/setuptools/releases)
- [Changelog](https://github.com/pypa/setuptools/blob/main/CHANGES.rst)
- [Commits](pypa/setuptools@v54.2.0...v65.5.1)

---
updated-dependencies:
- dependency-name: setuptools
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Lau <lauwing@f8ffc25e8e59.ant.amazon.com>
* Adding tarfile member sanitization to extractall()

* fix formatting issues

* Refactored extract_tarfile functions and added unit/functional tests

Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com>
Co-authored-by: Mohamed Elasmar <71043312+moelasmar@users.noreply.github.com>
Co-authored-by: hnnasit <84355507+hnnasit@users.noreply.github.com>
Co-authored-by: Haresh Nasit <hnnasit@amazon.com>
Co-authored-by: Wing Fung Lau <4760060+hawflau@users.noreply.github.com>
…updates (#4476)

* Add in instructions on updating SAMCLI dependencies

* Update the command to be run to write to reproducible linux

* Highlight content using separate sections for requirement files

* Fix typos and reword for better readability

* Update development guide text
* chore: update readme with pypi license

* use gh rather than pypi
@tam116

tam116 commented Jan 5, 2023

Copy link
Copy Markdown
Contributor Author

I've added integration tests and reworked the code remove the shallow copy and recursion

@hawflau hawflau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for updating the PR. Just one small change request. Otherwise it's good!

Comment thread samcli/hook_packages/terraform/hooks/prepare/enrich.py Outdated
@tam116
tam116 requested review from hawflau and removed request for hnnasit and torresxb1 January 9, 2023 17:58

@hawflau hawflau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your contribution! LGTM!

@mndeveci mndeveci removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Jan 10, 2023
@mildaniel
mildaniel merged commit 771b1fb into aws:develop Jan 10, 2023
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.

9 participants