-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Multi-auth trait support #3233
Multi-auth trait support #3233
Conversation
Run pre-commit and add test updates
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #3233 +/- ##
===========================================
+ Coverage 93.12% 93.16% +0.03%
===========================================
Files 66 66
Lines 14252 14287 +35
===========================================
+ Hits 13272 13310 +38
+ Misses 980 977 -3 ☔ View full report in Codecov by Sentry. |
tests/functional/test_auth_config.py
Outdated
|
||
|
||
def assert_all_requirements_match(auth_config, message): | ||
if len(auth_config) > 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's usually best to avoid conditionals in tests if possible. Branching like this leads to subtle failures where we may no longer meet the conditional and now the tests "pass" but do nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving this comment unresolved as there is still an if statement in this test, but this one is gone. Now there is just the if statement above:
if operation_model.auth:
auth_operations.append([service, operation_model])
Any alternatives that I can come up with involve loosening the checks of this test for no benefit and checking many empty lists to confirm that they are indeed empty.
Co-authored-by: Nate Prewitt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple more small comments but otherwise I think this is looking good.
Change test to use a comprehension instead of a for loop Co-authored-by: Nate Prewitt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple minor items, but this should be fine to ship if we've validated against our downstream consumers.
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
* release-1.35.0: Bumping version to 1.35.0 Update endpoints model Update to latest models Multi-auth trait support (#3233)
* Multiauth Adds support for the new multi-auth trait that will allow a service or operation to specify a list of compatible authentication types --------- Co-authored-by: Nate Prewitt <[email protected]>
Adds support for the new 'auth' trait on our models that will allow a priority list of auth types for a service or operation