-
Notifications
You must be signed in to change notification settings - Fork 47
fix: replaced taskcluster rest api calls with package #741
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
base: main
Are you sure you want to change the base?
Conversation
ec75ed6
to
40c0435
Compare
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 is shaping up nicely, thanks!
I have some changes to request. Once you put up a new revision I'll pull this down locally and see if it causes any differences in the graph (both here and in Gecko).
40c0435
to
80451af
Compare
This reverts commit cfb77a0.
Thanks! I'll take a closer look at this tomorrow. I'll try and test it against |
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.
Thanks, I tested this patch both in Taskgraph and the Gecko repo and it looks like it's working as expected!
But get_retry_post_session
, _do_request
and _handle_artifact
are all unused now in taskcluster.py
. Can you remove these and associated tests please?
Also I think I changed my mind about leaving We can do a major version bump for this.. no big deal. |
826551a
to
2e7e853
Compare
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.
Thank you!
This is mostly cleanup, but there are two issues I neglected to catch in my first review:
- The
_handle_artifact
for yaml - The
.capitalize()
issue
src/taskgraph/util/taskcluster.py
Outdated
continue | ||
|
||
raise e | ||
continue |
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.
Are you sure the taskcluster client would return None
on a 404? I would have thought it would raise an exception still.. Though it would probably be from aiohttp rather than requests.
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.
(I assume you verified this? It's important to preserve the 404 behaviour here)
8b37e80
to
549adac
Compare
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.
Thanks! Sorry for the delay + additional iteration. Getting close :)
except HTTPError as e: | ||
if e.response.status_code != 404: | ||
raise | ||
else: |
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.
I might be wrong, but I think if public/label-to-taskid.json
is a 404
then it would raise an exception rather than return None
. So this else
clause is probably dead code.
src/taskgraph/docker.py
Outdated
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.
Did these type failures start happening as a result of your changes? Or do you get these errors even on main
? If the latter, I wonder if there's something you need to do locally to get the type checker working
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.
These happened because of my change because currently task_def could be a json or a responce if it fails
src/taskgraph/optimize/strategies.py
Outdated
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.
Ditto
src/taskgraph/util/taskcluster.py
Outdated
except requests.HTTPError as e: | ||
task_def = get_task_definition(task_id) | ||
except Exception as e: | ||
raise e |
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.
Re-raising the exception without doing anything is the same as not catching it in the first place. So you might as well remove the try/except block.
src/taskgraph/util/taskcluster.py
Outdated
continue | ||
|
||
raise e | ||
continue |
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.
(I assume you verified this? It's important to preserve the 404 behaviour here)
fixes #221 . Created a bunch of helpers to use this package in taskcluster.py . I also didnt completely depcreate the url creation as that is still used. I bypassed it and replaced all uses when to get request is made.