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

feat: Extend AWS provider schema to support Fn::ToJsonString in environment variable #11461

Merged
merged 1 commit into from Oct 31, 2022
Merged

feat: Extend AWS provider schema to support Fn::ToJsonString in environment variable #11461

merged 1 commit into from Oct 31, 2022

Conversation

jurriaanpro
Copy link
Contributor

Closes: #11409

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Base: 85.79% // Head: 85.79% // No change to project coverage 👍

Coverage data is based on head (204a39f) compared to base (d2b6926).
Patch has no changes to coverable lines.

❗ Current head 204a39f differs from pull request most recent head 94272be. Consider uploading reports for the commit 94272be to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11461   +/-   ##
=======================================
  Coverage   85.79%   85.79%           
=======================================
  Files         314      314           
  Lines       13259    13259           
=======================================
  Hits        11375    11375           
  Misses       1884     1884           
Impacted Files Coverage Δ
lib/plugins/aws/provider.js 94.67% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ronkorving
Copy link
Contributor

ronkorving commented Oct 21, 2022

First of all, thank you so much for this PR! I'm really looking forward to seeing this land.

This may be out of scope, and not something I really need myself. But just wanted to raise it as a topic of interest. Environment variables inevitably will not be the only place where this will be useful.

For example, when crafting a CloudWatch Dashboard under Resources, ToJsonString would allow people to use YAML to define a dashboard and have it convert to an unwieldy JSON string through this method.

    MyDashboard:
      Type: AWS::CloudWatch::Dashboard
      Properties:
        DashboardName: MyDashboard
        DashboardBody: !ToJsonString
          etc:

This is the best example I could come up with, but can imagine there may be more places where people may want to use ToJsonString in their resources-section.

Or is this perhaps a non-issue because nothing is enforced under resources by Serverless?

@jurriaanpro
Copy link
Contributor Author

@ronkorving I think nothing is enforced under resources so long as you have a valid CloudFormation template. As mentioned in #11409, you'd need to include Transform: AWS::LanguageExtensions in your template as otherwise the Fn::ToJsonString won't be recognized/accepted.

Maybe someone that's deeper into the framework can confirm that.

@jurriaanpro
Copy link
Contributor Author

@medikoo Coverage failed, but on code I didn't touch. Is there something I can do to fix this?

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@jurriaanpro It looks good. Please see my comments

lib/plugins/aws/provider.js Outdated Show resolved Hide resolved
lib/plugins/aws/provider.js Outdated Show resolved Hide resolved
@jurriaanpro
Copy link
Contributor Author

@medikoo I've applied the suggested changes. Could you check again?

lib/plugins/aws/provider.js Outdated Show resolved Hide resolved
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@jurriaanpro can you rebase with main branch? I wonder why no github actions were not run - feels as GitHub quirk

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you!

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.

Allow environment variables based on structured objects, serialized with Fn::ToJsonString
3 participants