-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
[AppMesh] Routing #8012
[AppMesh] Routing #8012
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8012 +/- ##
==========================================
+ Coverage 94.39% 94.43% +0.03%
==========================================
Files 1130 1139 +9
Lines 96578 97321 +743
==========================================
+ Hits 91168 91903 +735
- Misses 5410 5418 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@bblommers this is ready for review. It doesn't completely close out #8001, but this was already fairly long so I thought I'd submit it as is and open a separate PR for the virtual_node APIs. |
tests/test_appmesh/test_appmesh.py
Outdated
tags=[{"key": "router_traffic", "value": "https"}], | ||
) | ||
router2 = connection.get("virtualRouter") | ||
assert router2 is not None |
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.
This assertion is technically not necessary, just because the next line immediately uses the variable. If the variable is None, the test will fail anyway with a KeyError
. There are quite a few examples like this.
For me, it would be cleaner to remove it - just because less lines of code make it easier to read.
It's very much personal preference though, and not particularly important, so I'm not expecting any changes. It's just an FYI - if you find it easier to read with the explicit assertions, feel free to keep it. 🙂
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.
LGTM - thank you @armichaud!
This is now part of moto >= 5.0.14.dev33 |
Implements the following methods for AppMesh