Add support for addon submission api

[eviljeff]

fixes #2451

Comments on this patch: It's large, but I couldn't see a way to split it into pieces. I attempted to add tests to cover the functionality in each function but I probably missed a lot of the detail. There are also two big things I realised was missing when I was writing the tests that will need to be addressed - I guess in follow-ups because this is already a lot for a single pr - before it's useful for end-users:

• The existing signing API has the ability to specify a guid for an add-on in the PUT url and it will be used to create the new add-on with that GUID. The submission API requires that if a guid is specified in the url it is in the manifest. So web-ext will need to patch the manifest in the xpi that is uploaded beforehand. The code in sign.js also saves and then uses the guid from the first submission to be able to submit subsequent versions under the same guid - the same limitation would apply here - it must be in the manifest. (The normal way of submitting extra versions to the same add-on, that wouldn't require the guid be specified in the manifest, is submitting to a different endpoint, POSTing to /addons/addon/<addon-id>/versions/, but a) web-ext doesn't have a way of the user specifying "this is a new add-on" vs. "this is a new version" and b) the patch I'm submitting doesn't support that endpoint currently.)
• There's no support for specifying additional metadata via the command line, nor tests for it being passed down to the relevant functions. As you need to provide extra metadata to create the first listed version on an add-on (or a new add-on with a listed version) - at least categories and version.license - this means anyone trying to use it for listed versions would be unable to.

[eviljeff]

I think it's ready for review(!)

Two of the tests I added I just couldn't get working - I marked them .skip -ideas welcome! Also now CI is failing in two of the jobs (but not all of them) with fsevents not accessible from chokidar 🤷

[eviljeff]

(Also, comment#0 updated)

[rpl]

I think it's ready for review(!)

Two of the tests I added I just couldn't get working - I marked them .skip -ideas welcome! Also now CI is failing in two of the jobs (but not all of them) with fsevents not accessible from chokidar shrug

I just tried to pinpoint how the package-lock.json got to trigger that unexpected error, I believe that may have been due to a previous version of this branch having npm installed as a dependency.

That may have left some traces into your package-lock.json and it ended up into this odd inconsistent state.

I'm not sure how technically those have been left there after you removed the dependency, I'd need to know the exact sequence of steps that you did locally when you removed the npm dependency.

Anyway, in the past I had my share of issues with the package-lock.json entering odd corner cases, from what I recall at the time I got the impression that a previous package-lock.json file or node_modules dir may affect the changes applied to the package-lock.json file from the exact same pacakge.json file.

And so, to be 100% sure I'm always recomputing the package-lock.json in a reproducible way, when I rebuilt the lock file locally I usually:

1. remove the package-lock.json and node_modules dir first
2. and then I run npm install to recreate it (or npx npm@SOMEVERSION install if I need to be sure a certain npm version is used to recompute the lock file)

I tried exactly that locally and that was enough to make npm ci to work as expected on npm v7 (which was the version I was using locally with node v14 by default, and it was able to trigger the exact same issue as the failed CI jobs).

I also merged some PR updating the package.json and package-lock.json file today, and so I also tried that again after rebasing this branch on today's master branch and the conflict on the package-lock.json file was also the only conflict to resolve in the rebase and so I added a copy of the fixed package-lock.json file I got locally in this gist (the gist is long and so you may not notice the second file, but you can clone the gist locally and get the one you want to try in this branch, with the one I created after rebasing you could also rebase the branch which may also be good):

• https://gist.github.com/rpl/7ed9480e5f21bcf7b29cd8d2e8c418c6

@eviljeff would you mind to give that a try and update this PR so that we can confirm that the CI job failures are going to be fixed with the package-lock.json files recreated with the approach described above?

[rpl]

I took a look to the windows CI job failure, as I mentioned yesterday I was pretty sure to have seen that and fixed it not long ago, and confirmed that it has been already fixed in mozilla/web-ext#2444 along with the dependency update that started to trigger it, in particular it has been fixed by this change here:

https://github.com/mozilla/web-ext/blob/5e3b2144e7be25c9553c8f4abc3b98d0b6d9a8a8/tests/functional/common.js#L101-L119

The reason why the fixed failure came back seems to be due to the fact that the package.json file may have been updated manually to match how it is in 7.1.1 (after mozilla/web-ext#2444 was merged), but the fix in tests/functional/common.js isn't there because the branch has not be rebased.

e.g. I can see it being missing here:

https://github.com/mozilla/web-ext/blob/df79392d38d01810529db6b38ad6d57d38ecf588/tests/functional/common.js#L101-L108

@eviljeff do you confirm this is the case? (the package.json manually aligned to the one in 7.1.1 but the rest of the working tree still branched from 7.1.0)

[eviljeff]

I took a look to the windows CI job failure, as I mentioned yesterday I was pretty sure to have seen that and fixed it not long ago, and confirmed that it has been already fixed in #2444 along with the dependency update that started to trigger it

I copied over the changed lines and it's working now 👍 (I did try to cherry-pick the whole commit, but package-lock.json 🤷‍♂️ )

[eviljeff]

Also, I am trying to see how we could avoid the Promise + setTimeout soup in the waitForXXX methods because that really isn't great (legacy from sign-addon I suppose).

I had a try, but the need to reject/resolve from functions inside the timeout seems to stop us from removing the new Promise and just relying on standard return xxx and throw instead. I'm now looking into alternatives that would support the delay+retry functionality further up the stack.

[eviljeff]

Also, I am trying to see how we could avoid the Promise + setTimeout soup in the waitForXXX methods because that really isn't great (legacy from sign-addon I suppose).

I had a try, but the need to reject/resolve from functions inside the timeout seems to stop us from removing the new Promise and just relying on standard return xxx and throw instead. I'm now looking into alternatives that would support the delay+retry functionality further up the stack.

The packages I found that said they added retry to fetch were more for when the request encountered http errors (non-200, etc, error codes) rather than arbitrary retry when we decide the exact json response isn't what we want. So I refactored the retry functionality into a separate function - it doesn't relieve us from the Promise + setTimeout soup but does at least split it up somewhat, and allow validation & approval steps to share some code.