fix: added support for taskcluster-taskgraph v16.2.1 #117
Conversation
|
Looks like maybe some test or mocking changes are needed here? |
| method="GET", | ||
| url=get_artifact_url( | ||
| list(previous_graphs.keys())[0], "public/parameters.yml" | ||
| list(previous_graphs.keys())[0], "public%2Fparameters.yml" |
There was a problem hiding this comment.
That looks very wrong, callers shouldn't have to urlencode artifact names.
There was a problem hiding this comment.
The getLatestArtifact encodes URL by default which is I added it here to catch the response, I was also testing this taskcluster-taskgraph and it does encode it there as well
There was a problem hiding this comment.
This is the input url to getLatestArtifact though, not an output from it. (And this is at least part of the reason you have failing tests. The code being tested here does not url encode the input to this.
There was a problem hiding this comment.
with the testing I have done with getLatestArtifact, I have noticed that when it builds the URL to make the api call in the taskcluster package, it encodes the name before making the request so to try can catch that request, I mocked the encoded version. This also was not present before as we would construct the url directly instead of using the taskcluster package
There was a problem hiding this comment.
It looks like the difference here is that the old get_artifact code didn't do any URL encoding before sending the request off, whereas the taskcluster client does.
abhishekmadan30
force-pushed
the
upgrade
branch
7 times, most recently
from
1ba8a0b to
f356fb6
Compare
abhishekmadan30
changed the title
test/transforms/test_replicate.py
Outdated
| from mozilla_taskgraph.transforms.replicate import transforms as replicate_transforms | ||
|
|
||
| TC_ROOT_URL = "https://tc-tests.example.com" | ||
| TC_ROOT_URL = os.environ.get("TASKCLUSTER_PROXY_URL", "https://tc-tests.example.com") |
There was a problem hiding this comment.
Is there a reason we need to support overriding this in the environment? These get mocked out anyways, so there should never be a reason for them to be a real proxy URL?
(If this were non-test code this would probably be needed...but in test code I can't see why we'd want it.)
There was a problem hiding this comment.
I think it does get set as now by default if "TASKCLUSTER_PROXY_URL" is found in the environment, it would use that as the URL as we removed the use_proxy. With this I think there are 2 options, we could keep these implementation or I could force clear the "TASKCLUSTER_PROXY_URL" before running these tests
There was a problem hiding this comment.
Ah; in that case I think it would be preferable to forcibly override this environment variable, to ensure that we're always using the value specified here. (With the current code, I would expect this to be the real tc proxy when running in tc. This probably wouldn't cause issues most of the time, but eventually I imagine it will cause some confusion.)
There was a problem hiding this comment.
oh ok sounds good, I made that change !
| method="GET", | ||
| url=get_artifact_url( | ||
| list(previous_graphs.keys())[0], "public/parameters.yml" | ||
| list(previous_graphs.keys())[0], "public%2Fparameters.yml" |
There was a problem hiding this comment.
It looks like the difference here is that the old get_artifact code didn't do any URL encoding before sending the request off, whereas the taskcluster client does.
abhishekmadan30
force-pushed
the
upgrade
branch
3 times, most recently
from
3b7e0f4 to
7682cb8
Compare
| does_not_raise(), | ||
| id="shipit_valid", | ||
| ), | ||
| pytest.param( |
There was a problem hiding this comment.
Is this an accidental removal?
There was a problem hiding this comment.
No as we update graph config schema to allow extra parameters not defined in the schema, somewhere in taskgraph 12 and I just updating this to ensure this test doesn't fail once we swap over to 16.2. So this test just became redundant so I though it would be best to remove it
There was a problem hiding this comment.
abhishekmadan30
force-pushed
the
upgrade
branch
from
7682cb8 to
021bdae
Compare
ahal
left a comment
ahal
left a comment
There was a problem hiding this comment.
Thanks, this looks good to me!
4 participants
No description provided.