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

Enhance CompatibleRuntimes property of AWS::Serverless::LayerVersion #810

Closed
wants to merge 4 commits into from
Closed

Enhance CompatibleRuntimes property of AWS::Serverless::LayerVersion #810

wants to merge 4 commits into from

Conversation

systemdesignpartners
Copy link

Allow it to accept a !Ref to a String whereas currently it requires a List

Bugfix for Issue #809

Hopefully this addresses the minor enhancement I suggested in #809, which is to allow CompatibleRuntimes to be supplied by a String Parameter instead of insisting on a List. My first time looking at this codebase so buyer beware :)
Thanks
Tony

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

@codecov-io
Copy link

codecov-io commented Feb 13, 2019

Codecov Report

Merging #810 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           develop    #810   +/-   ##
=======================================
  Coverage     94.8%   94.8%           
=======================================
  Files           69      69           
  Lines         3079    3079           
  Branches       580     580           
=======================================
  Hits          2919    2919           
  Misses          84      84           
  Partials        76      76
Impacted Files Coverage Δ
samtranslator/model/lambda_.py 88.46% <ø> (ø) ⬆️
samtranslator/model/sam_resources.py 95.5% <ø> (ø) ⬆️

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 68d1ed7...c67f9b5. Read the comment docs.

Copy link
Contributor

@brettstack brettstack left a comment

Choose a reason for hiding this comment

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

Please delete the .swp files also

samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
samtranslator/model/sam_resources.py Outdated Show resolved Hide resolved
tests/translator/input/layers_with_intrinsics.yaml Outdated Show resolved Hide resolved
tests/translator/output/aws-cn/layers_with_intrinsics.json Outdated Show resolved Hide resolved
tests/translator/input/layers_with_intrinsics.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

Could you submit this PR against the develop branch? Thanks

@systemdesignpartners
Copy link
Author

Branch is now up to date with 'origin/develop'

@jlhood
Copy link
Contributor

jlhood commented Feb 22, 2019

@systemdesignpartners If you rebased your changes onto develop locally, you'll need to push them to your remote in order for them to be reflected in this PR. Also, please change the base on this PR to point to develop branch instead of master as well (click edit button at the top of the PR to select the base branch).

@systemdesignpartners systemdesignpartners changed the base branch from master to develop February 22, 2019 20:19
@systemdesignpartners
Copy link
Author

Should be good on the rebase to develop. I added several test cases but unfortunately could not get some nice-to-haves to work, such as supplying a list of !Refs to CompatibleRuntimes, so I left it out.

@keetonian
Copy link
Contributor

Hey! Thanks for changing the base branch on this PR. Could you also rebase onto develop to resolve merge commits and remove some of the commits that are on master but not on develop?

Here are the commands to run:

git remote add upstream https://github.com/awslabs/serverless-application-model.git
git fetch upstream
git rebase upstream/develop 
git push -f

If you run into conflicts on rebase, here are additional instructions on how to resolve it: https://help.github.com/en/articles/resolving-merge-conflicts-after-a-git-rebase

@systemdesignpartners
Copy link
Author

systemdesignpartners commented Feb 26, 2019 via email

@jlhood
Copy link
Contributor

jlhood commented Feb 26, 2019

@systemdesignpartners Thanks for the quick follow up! You rebased correctly, but your changes are failing in the CI build. If you run make pr it'll show you the failures to fix. Make the changes necessary to fix the failures, commit and push again to run the CI build again.

@praneetap praneetap requested a review from keetonian June 13, 2019 19:11
@atreyd
Copy link

atreyd commented Aug 5, 2019

Any idea by when these changes will be rolled out to SAM. ?

Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

Could you add a new section to the test that uses your new parameters and update the test outputs? This looks good otherwise 👍

  LayerWithRuntimesList:
    Type: 'AWS::Serverless::LayerVersion'
    Properties:
      ContentUri: s3://sam-demo-bucket/layer.zip
      CompatibleRuntimes:
        - !Ref LayerRuntimeString1
        - !Ref LayerRuntimeString2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants