1242877 - taskcluster-npm-cache supports using newer taskcluster-client for buildURL API

Status: RESOLVED FIXED

(Taskcluster :: Services, defect, P1)

Description

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

+++ This bug was initially created as a follow-up of Bug #1238469 Comment #38 +++

In https://github.com/taskcluster/taskcluster-client/commit/b0f7e20fd34adc3d4b0cf596dd9531fbd2014815, taskcluster-client buildUrl API formalizes the parameter, so that 'public/node_modules.tar.gz'[1] is formalized to 'public%2Fnode_modules.tar.gz'. Look like we need to separated `public` and `node_modules.tar.gz` as two parameters in taskcluster-npm-cache-get.js
, e.g.

  let url = await queue.buildUrl(
    queue.getLatestArtifact,
    indexedTask.taskId,
    'public',
    'node_modules.tar.gz'
  );

[1] https://github.com/taskcluster/npm-cache/blob/52ecb6bbc44ebd6f16b69e97cb2c9476fd7f012d/src/bin/taskcluster-npm-cache-get.js#L90

Comment 1

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

Attachment: [npm-cache:master] PR#6, taskcluster-npm-cache supports using newer taskcluster-client for buildURL API

Comment 2

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

Comment on attachment 8712044 [details] [review]
[npm-cache:master] PR#6, taskcluster-npm-cache supports using newer taskcluster-client for buildURL API

Hi Aus, may I have your review? Thank you.

Comment 3

[Aus Lacroix [:aus]]

Commit: https://github.com/taskcluster/npm-cache/commit/9305605e389b91c728d04e7f80bcba651b31dfd0

I'll file a bug to release new docker worker images based on this new npm-cache client.

Comment 4

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> +++ This bug was initially created as a follow-up of Bug #1238469 Comment
> #38 +++
> 
> In
> https://github.com/taskcluster/taskcluster-client/commit/
> b0f7e20fd34adc3d4b0cf596dd9531fbd2014815, taskcluster-client buildUrl API
> formalizes the parameter, so that 'public/node_modules.tar.gz'[1] is
> formalized to 'public%2Fnode_modules.tar.gz'. Look like we need to separated
> `public` and `node_modules.tar.gz` as two parameters in
> taskcluster-npm-cache-get.js
> , e.g.
> 
>   let url = await queue.buildUrl(
>     queue.getLatestArtifact,
>     indexedTask.taskId,
>     'public',
>     'node_modules.tar.gz'
>   );
> 
> [1]
> https://github.com/taskcluster/npm-cache/blob/
> 52ecb6bbc44ebd6f16b69e97cb2c9476fd7f012d/src/bin/taskcluster-npm-cache-get.
> js#L90

I found another problem: getLatestArtifact takes only two arguments [1] :(

Hi :aus, :jonasf, could you help to provide some advice on this? Thank you.

https://github.com/taskcluster/taskcluster-client/blob/master/lib/apis.js#L1381-L1398

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

It takes taskId and artifact name.
Hint, the artifact folder is part of the artifact name:

>   let url = await queue.buildUrl(
>     queue.getLatestArtifact,
>     indexedTask.taskId,
>     'public/node_modules.tar.gz'
>   );

See: http://docs.taskcluster.net/queue/api-docs/#getLatestArtifact

I hope that helps.

Comment 6

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

Attachment: [npm-cache:master] PR#7, Rename the downloaded file to node_modules.tar.gz

Comment 7

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

Comment on attachment 8713101 [details] [review]
[npm-cache:master] PR#7, Rename the downloaded file to node_modules.tar.gz

Separating the parameter doesn't work because the
getLatestArtifact() takes only two arguments [1]. We still need to use
public/node_modules.tar.gz for artifact name. Due to buildUrl() formalizes
the name to public%2Fnode_modules.tar.gz, we have to rename the downloaded
file to node_modules.tar.gz in order to make it can be extracted correctly.

This time, I tested it with packaging, installing the changed code in tester image and running gaia test locally. I am pretty sure this patch fixes the problem.

May I have your review again, Aus? Thank you. 

[1] http://docs.taskcluster.net/queue/api-docs/#getLatestArtifact

Comment 8

[Aus Lacroix [:aus]]

Comment on attachment 8713101 [details] [review]
[npm-cache:master] PR#7, Rename the downloaded file to node_modules.tar.gz

Wow, that's ummm... weird. Well, it will be good to see this fixed, it should get rid of intermittent issues we're seeing with npm-cache currently.

Comment 9

[Aus Lacroix [:aus]]

Commit: https://github.com/taskcluster/npm-cache/commit/b25e2ff381ea700edb8b188650123451ec7a63f0

+ taskcluster-npm-cache@1.3.3 published.

Thanks Edgar! :)

Comment 10

[Edgar Chen [:edgar] (OOO untils 09/01, slow response)]

Thank you, Aus!

Comment 11

[Pete Moore [:pmoore][:pete]]

Moving closed bugs across to new Bugzilla product "TaskCluster".