Conversation
hneiva
left a comment
hneiva
left a comment
There was a problem hiding this comment.
Just a minor change, but LGTM otherwise.
src/taskgraph/util/verify.py
Outdated
| if route.startswith(route_prefix): | ||
| # Check for invalid characters in the route | ||
| if "/" in route: | ||
| print(f"DEBUG: Found slash in route: {route}") |
There was a problem hiding this comment.
use logger.debug or logger.error instead of print
hneiva
left a comment
hneiva
left a comment
There was a problem hiding this comment.
Oops, didn't notice tests are failing
bhearsum
left a comment
bhearsum
left a comment
There was a problem hiding this comment.
This is a good start, but it only covers the subset of cases that have the trust domain or notification prefixes. Eg: if you add a test with some other route, I expect it fail (I encourage you to add this before addressing the problem to double check me!).
Because this is meant to be a general check, you should add a brand new function with the verifications.add("full_task_graph") decorator, and check all routes. (Which means you'll be able to remove the checks you added to the existing decorators.)
|
Excellent work, thank you! |
|
Ah; it looks like the checks I had to approve to run caught a linting issue. Once that's fixed we can get this merged. |
src/taskgraph/util/verify.py
Outdated
|
|
||
| for route in routes: | ||
| # Check for invalid / in the route | ||
| if "/" in route: |
There was a problem hiding this comment.
Sorry for the late request.. but it just occurred to me that only index routes can't have a slash. Routes in general are allowed to have a slash.
So we should change this to if route.startswith("index.") and "/" in route:. Also updating the function name to verify_index_routes.. or similar.
ahal
force-pushed
the
add-exception-for-slash-in-route
branch
from
fca7df2 to
e449dae
Compare
4 participants
Makes an exception be thrown if a route contains a "/" and added tests for it. Closes #254