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
Some verification was added in taskcluster#722 but for slashes only. Taskcluster is actually way stricter than that with what it accepts as index keys and it doesn't prevent task creations when index routes are invalid (taskcluster/taskcluster#6266). The regex comes from the service iteself at https://github.com/taskcluster/taskcluster/blob/62655d84d55680ffb8faa35e7deadee69f389efa/services/index/src/helpers.js#L8 Fixes taskcluster#883
Some verification was added in taskcluster#722 but for slashes only. Taskcluster is actually way stricter than that with what it accepts as index keys and it doesn't prevent task creations when index routes are invalid (taskcluster/taskcluster#6266). The regex comes from the service iteself at https://github.com/taskcluster/taskcluster/blob/62655d84d55680ffb8faa35e7deadee69f389efa/services/index/src/helpers.js#L8 Fixes taskcluster#883
Some verification was added in #722 but for slashes only. Taskcluster is actually way stricter than that with what it accepts as index keys and it doesn't prevent task creations when index routes are invalid (taskcluster/taskcluster#6266). The regex comes from the service iteself at https://github.com/taskcluster/taskcluster/blob/62655d84d55680ffb8faa35e7deadee69f389efa/services/index/src/helpers.js#L8 Fixes #883
4 participants
Makes an exception be thrown if a route contains a "/" and added tests for it. Closes #254