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

Fix Issue #717 Cannot set DefaultAuthorizer and have CORS enabled #958

Merged
merged 12 commits into from
Jun 14, 2019
Merged

Fix Issue #717 Cannot set DefaultAuthorizer and have CORS enabled #958

merged 12 commits into from
Jun 14, 2019

Conversation

xrendan
Copy link
Contributor

@xrendan xrendan commented Jun 6, 2019

Issue #717

Description of changes:
Add AddDefaultAuthorizerToCorsPreflight parameter Auth object so that you can in API fixed code from PR #828 so that it does not fail the test cases.

Description of how you validated changes:
Generated test cases for default condition and with AddDefaultAuthorizerToCorsPreflight set to False, the output cloudformation json has security for the options method when it is True and has no security when it is set to False.

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation

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

@xrendan
Copy link
Contributor Author

xrendan commented Jun 6, 2019

The tests failed due to API gateway hashes and I don't know how to fix that and I don't know how to generate test cases for aw-us-gov and aws-cn so I need direction or help on that.

@praneetap
Copy link
Contributor

The tests failed due to API gateway hashes and I don't know how to fix that and I don't know how to generate test cases for aw-us-gov and aws-cn so I need direction or help on that.

To fix the hashes could you try running the tests in python 2.7 and copying the output into the output/json, output/aws-cn/json and output/aws-us-gov/json files.
Generating test cases for aws-cn and aws-us-gov should happen automatically for your tests. You will see the output for those json files in the error output and would just need to copy over to the right path.

@keetonian keetonian changed the base branch from master to develop June 11, 2019 17:50
@praneetap
Copy link
Contributor

@xrendan I fixed the tests and created a pr against your branch. Can you please merge it in? I don't have permissions to do so.

@xrendan
Copy link
Contributor Author

xrendan commented Jun 12, 2019

I merged the PR for you, is there anything else that I need to do?

samtranslator/swagger/swagger.py Outdated Show resolved Hide resolved
tests/translator/test_translator.py Outdated Show resolved Hide resolved
@praneetap
Copy link
Contributor

I merged the PR for you, is there anything else that I need to do?

@xrendan That would be it! Thanks. That should fix the build issues!

@keetonian
Copy link
Contributor

Guess that didn't do it; I made the changes locally this time and submitted another PR to your branch: https://github.com/xrendan/serverless-application-model/pull/2

That should fix the test failures. Sorry about this!

@codecov-io
Copy link

codecov-io commented Jun 13, 2019

Codecov Report

Merging #958 into develop will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #958      +/-   ##
===========================================
- Coverage     94.8%   94.69%   -0.11%     
===========================================
  Files           69       69              
  Lines         3079     3149      +70     
  Branches       580      604      +24     
===========================================
+ Hits          2919     2982      +63     
- Misses          84       85       +1     
- Partials        76       82       +6
Impacted Files Coverage Δ
samtranslator/model/api/api_generator.py 93.58% <100%> (-1.36%) ⬇️
samtranslator/swagger/swagger.py 97.66% <100%> (-0.11%) ⬇️
samtranslator/model/sam_resources.py 95.5% <0%> (ø) ⬆️
samtranslator/model/eventsources/push.py 88.85% <0%> (ø) ⬆️

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...d9de066. Read the comment docs.

@xrendan
Copy link
Contributor Author

xrendan commented Jun 13, 2019

@keetonian it's all merged up, thanks for fixing my tests, I hadn't had the time to do it myself with work this week. I'll submit a PR for the unbound local issue later today or tomorrow.

@jlhood jlhood mentioned this pull request Jun 14, 2019
samtranslator/swagger/swagger.py 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.

Looks good! Thank you for the contribution!

@keetonian keetonian merged commit 35416f6 into aws:develop Jun 14, 2019
@leantorres73
Copy link

leantorres73 commented Jun 14, 2019

Maybe this is a silly question but I don't know the workflows. When is this change going to be available in AWS CLI? @keetonian

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.

6 participants