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 overriding security spec with empty list #121

Merged
merged 1 commit into from
Sep 29, 2016
Merged

Allow overriding security spec with empty list #121

merged 1 commit into from
Sep 29, 2016

Conversation

jlumpe
Copy link
Contributor

@jlumpe jlumpe commented Sep 27, 2016

Currently the global security property of the spec can be overridden in specific operations, but only if the list of security requirements is non-empty.

In the official spec it states that

To remove a top-level security declaration, an empty array can be used

for an operation's security property.

This change should bring things in line with the official spec.

@coveralls
Copy link

coveralls commented Sep 27, 2016

Coverage Status

Coverage remained the same at 97.34% when pulling d8f7684 on jlumpe:master into 14de09f on Yelp:master.

@macisamuele
Copy link
Collaborator

@jlumpe thanks a lot for your contribution and for having identified the specs un-compliant behaviour.

Your change looks correct to me but I would ask if you can add a couple of tests in security_object_test.py that validates your change.

@coveralls
Copy link

coveralls commented Sep 28, 2016

Coverage Status

Coverage remained the same at 97.34% when pulling 5a10c81 on jlumpe:master into 20e685d on Yelp:master.

@coveralls
Copy link

coveralls commented Sep 28, 2016

Coverage Status

Coverage remained the same at 97.34% when pulling b09ce6a on jlumpe:master into 20e685d on Yelp:master.

@jlumpe
Copy link
Contributor Author

jlumpe commented Sep 28, 2016

Updated with new test. Test passes this branch, fails on master. Hope I got the code style right.

@macisamuele
Copy link
Collaborator

@jlumpe: thanks for the tests ... with them we could be sure that the code change is doing what is expected.

@macisamuele macisamuele merged commit 380e2b6 into Yelp:master Sep 29, 2016
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.

3 participants