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

Allow Api.Auth in Globals section #682

Merged
merged 12 commits into from
Jan 22, 2019
Merged

Allow Api.Auth in Globals section #682

merged 12 commits into from
Jan 22, 2019

Conversation

sjawhar
Copy link
Contributor

@sjawhar sjawhar commented Nov 24, 2018

#512 added the ability to set API auth, and the original issue outlined including in the Globals section. However, it looks like the Globals checker actually is excluding Auth as one of the supported properties. Is there a reason that is so? I figured opening a PR was a quick and easy way to start a discussion as well as move toward a fix. Looking forward to your feedback!

@keetonian keetonian changed the base branch from master to develop November 27, 2018 20:29
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.

Thank you for submitting this! Could you review why the tests aren't passing and fix them?

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #682   +/-   ##
========================================
  Coverage    94.17%   94.17%           
========================================
  Files           67       67           
  Lines         2678     2678           
  Branches       478      478           
========================================
  Hits          2522     2522           
  Misses          80       80           
  Partials        76       76
Impacted Files Coverage Δ
samtranslator/plugins/globals/globals.py 100% <ø> (ø) ⬆️

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

@sjawhar
Copy link
Contributor Author

sjawhar commented Dec 10, 2018

No problem, it was an expected error message that needed updating. This doesn't necessarily address the need for new tests, though. If you think they are needed, please point me to where I can get started.

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.

Hi! Thanks for fixing that! Could you also add a success-case test? It should be as simple as adding an Auth section (see example in this test) to the globals-for-api test, then fixing the test output to match the new expected behavior.

@sjawhar
Copy link
Contributor Author

sjawhar commented Dec 14, 2018

I've added the Auth section to the globals_for_api test. Now it looks like the test is failing because unicode is causing the match assertion to fail. Here's a snippet:
image

Any tips?

@keetonian
Copy link
Contributor

I'll take a look, I haven't seen this before.

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.

I updated the tests- needed to change some of the hashes on the deployments as well as add in the security sections

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.

Tests pass in py36 but not py27. Investigating

@jlhood jlhood requested review from brettstack and removed request for jlhood January 8, 2019 18:28
@jlhood
Copy link
Contributor

jlhood commented Jan 8, 2019

@brettstack Could you review this one? Just wanted to sanity check that you hadn't intentionally excluded Api.Auth from globals for a reason.

@brettstack brettstack merged commit 292b19d into aws:develop Jan 22, 2019
@cadetstar
Copy link

Is there an ETA for this getting published and available for use? Alternately, can I override the branch aws-sam-cli uses locally to validate the template?

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.

None yet

6 participants