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: Add support for the FailOnWarnings property on AWS::Serverless::Api #2417

Merged
merged 7 commits into from Jul 6, 2022
Merged

feat: Add support for the FailOnWarnings property on AWS::Serverless::Api #2417

merged 7 commits into from Jul 6, 2022

Conversation

nialdaly
Copy link
Contributor

@nialdaly nialdaly commented Jun 17, 2022

Issue

Description of changes:

Description of how you validated changes:

Checklist:

  • Add/update unit tests using:
  • Add/update integration tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected

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

@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 Jun 17, 2022
@nialdaly nialdaly changed the title feat: Add support for the FailOnWarnings property feat: Add support for the FailOnWarnings property on AWS::Serverless::Api Jun 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2022

Codecov Report

Merging #2417 (3e88c04) into develop (e7a1496) will increase coverage by 0.85%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2417      +/-   ##
===========================================
+ Coverage    93.58%   94.44%   +0.85%     
===========================================
  Files           90       98       +8     
  Lines         6124     7269    +1145     
  Branches      1260     1512     +252     
===========================================
+ Hits          5731     6865    +1134     
- Misses         183      195      +12     
+ Partials       210      209       -1     
Impacted Files Coverage Δ
samtranslator/region_configuration.py 77.77% <0.00%> (-22.23%) ⬇️
samtranslator/model/codedeploy.py 90.90% <0.00%> (-9.10%) ⬇️
samtranslator/validator/validator.py 91.80% <0.00%> (-3.85%) ⬇️
samtranslator/model/exceptions.py 97.67% <0.00%> (-2.33%) ⬇️
samtranslator/open_api/open_api.py 90.16% <0.00%> (-1.81%) ⬇️
samtranslator/model/s3_utils/uri_parser.py 68.42% <0.00%> (-0.81%) ⬇️
samtranslator/yaml_helper.py 89.47% <0.00%> (-0.53%) ⬇️
samtranslator/translator/logical_id_generator.py 90.62% <0.00%> (-0.29%) ⬇️
samtranslator/model/api/api_generator.py 94.11% <0.00%> (-0.25%) ⬇️
samtranslator/model/apigateway.py 96.98% <0.00%> (-0.18%) ⬇️
... and 46 more

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 30f206a...3e88c04. Read the comment docs.

@nialdaly nialdaly marked this pull request as ready for review June 17, 2022 18:23
@mildaniel
Copy link
Contributor

mildaniel commented Jun 21, 2022

Hi @nialdaly, thank you for submitting a pull request! Can you please add unit and integration tests to your PR so that we can validate these changes and guard against regressions in the future.

@nialdaly
Copy link
Contributor Author

nialdaly commented Jun 21, 2022

No problem! @mildaniel Sorry I forgot to put a comment here. I'm unsure on how to generate an API creation warning to ensure that the rollback occurs with FailOnWarnings: true as described in the documentation. Would you have any advice on that?

@hawflau hawflau requested review from a team, jfuss and mingkun2020 and removed request for a team June 28, 2022 03:07
@torresxb1 torresxb1 removed the stage/needs-triage Automatically applied to new issues and PRs, indicating they haven't been looked at. label Jun 28, 2022
@hawflau
Copy link
Contributor

hawflau commented Jun 28, 2022

I'm unsure on how to generate an API creation warning to ensure that the rollback occurs with FailOnWarnings: true as described in the documentation.

@nialdaly, sorry I don't quite get what you mean. If you want to make sure a stack/API gets rollback due to "FailOnWarnings: true", this has to been done when the stack is being deployed. SAM Transform happens before the stack is deployed.

Copy link
Contributor

@hawflau hawflau left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just one comment. And thanks for your contribution!

@nialdaly nialdaly requested a review from hawflau June 29, 2022 23:03
Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, looks good to me as well!

@mndeveci mndeveci merged commit 5f7c1ce into aws:develop Jul 6, 2022
@nialdaly nialdaly deleted the feature/fail-on-warnings branch July 7, 2022 18:46
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.

Feature request: Support FailOnWarnings for AWS::Serverless::Api
8 participants