feat: Automatically generate updateManifest files from previous updateManifests #552

Azeirah · 2016-10-03T19:34:19Z

This pull request is not ready. Tests need to be written, documentation needs to be updated, and a short code-review would be really helpful. I've done some ad-hoc testing with this command

Usage and testing:

This feature expects a manifest.json to exist. For it to generate a new updateManifest, an older one needs to exist on a web server somewhere. This is configured in the manifest.json's applications.gecko.update_url property. For my ad-hoc tests I've simply used my personal web-server.

The command that I used

./web-ext sign --api-key {YOUR-USER} --api-secret {YOUR-API} --api-url-prefix https://addons-dev.allizom.org/api/v3 --update-link https://example.com/{xpiFileName}

What I did

What still needs to be done:

Automated tests
Code review
Additions to the official documentation

Azeirah · 2016-10-03T19:35:03Z

src/cmd/sign.js

        downloadDir: artifactsDir,
      });

+      if (updateLink && updateLink.length > 0 && signingResult.success) {


updateLink.length > 0 is unnecessary, overlooked this one

Azeirah · 2016-10-03T19:36:42Z

src/cmd/sign.js

+          if (error) {
+            reject(error);
+          }
+          resolve();


Should probably return something, {success: Boolean, newUpdateManifestPath: String, ...?}

kumar303 · 2016-10-04T04:02:09Z

src/cmd/sign.js

+          extensionID = id;
+        }
+        await generateNewUpdateManifest(
+          extensionID, manifestData.version, signingResult.downloadedFiles[0],


Keep in mind that signingResult.downloadedFiles is an array of many files. I'm not entirely sure if newer WebExtensions will have multiple files but historically AMO would sign one file for desktop, one file for Android, and so on (this was mostly for old add-ons with binary components though). In any case, I suppose we should try to support an array of files?

I understand that's the case, but how would that fit inside an updateManifest.json file? Here's what I don't get:

id: 123456789
version: 1.2
android: someGeneratedXPI-ANDROID-v1.2.xpi
desktop: someGeneratedXPI-DESKTOP-v1.2.xpi

The older updateManifest.json file looks like this

{ "addons": { "123456789": { "updates": [] } } }

So let's update it with the given data

{ "addons": { "123456789": { "updates": [ { "version": "1.2", "update_link": "https://example.com/someGeneratedXPI-ANDROID-v1.2.xpi" }, { // also version 1.2? // this shouldn't be possible, right? // the android XPI has the same extensionID, no? "version": "1.2", "update_link": "https://example.com/someGeneratedXPI-DESKTOP-v1.2.xpi" } ] } } }

kumar303 · 2016-10-04T04:03:43Z

src/cmd/sign.js

+        `${manifestData.applications.gecko.update_url}`);
+    }
+
+    if (updateLink.indexOf('{xpiFileName}') === -1) {


All of these usage errors should happen at the start of the sign command so that developers see them immediately rather than having to wait until the signing process completes

kumar303 · 2016-10-04T04:05:54Z

src/cmd/sign.js

+
+    return new Promise(function(resolve, reject) {
+      log.info('Saving to', path.join(artifactsDir, updateManifestFileName));
+      fs.writeFile(


you can just await fs.writeFile() (it returns a promise since we're using mz/fs)

kumar303 · 2016-10-04T04:07:17Z

Looks like you're on the right track! Since there are a lot of error cases, I'd suggest breaking it into smaller functions so that it's easier to test.

Azeirah · 2016-10-06T15:59:57Z

Thanks for the feedback, I'm working on it!

…ns for testability

Azeirah · 2016-10-10T22:56:31Z

I've pushed a version where I split-up the function into three parts, @kumar303
~~However, now some of the existing tests are failing because of a timeout, I'm checking that out atm~~

Fixed that, it was because I removed a check that was there before.

Now tests need to be written, and some documentation. Unless you still have some remarks?

…tionality

kumar303

I took a quick look and requested some changes. I can take a closer look when you start adding tests.

I can do another review once you start adding tests if you just want some early feedback before finishing all tests. Thanks for all the follow-ups!

kumar303 · 2016-10-11T21:16:48Z

src/cmd/sign.js

+        let oldUpdateManifestDataMakeFlowHappy =
+          oldUpdateManifestData == null ? {} : oldUpdateManifestData;
+        await createNewUpdateManifest(
+          extensionID, signingResult.downloadedFiles[0], artifactsDir,


ok, I looked more into the case where multiple files could exist in signingResult.downloadedFiles and this is just for legacy (non-webextension) add-ons. So, yeah, it should never happen. However, I'd like to see something that checks the length of this array and throws an error if it's longer than one. The error should just explain how such a situation is not supported.

kumar303 · 2016-10-11T21:23:57Z

src/cmd/sign.js

+// eslint-disable-next-line
+async function createNewUpdateManifest(
+  id: ?string, XPIPath: string, artifactsDir,
+  manifestData: Object, updateLink: string, oldUpdateManifestData: Object


All arguments should be keyword args to make them more readable and easier to change later on. This is a simple change using ES6 destructors (just add braces):

async function createNewUpdateManifest({ id: ?string, XPIPath: string, artifactsDir, ... }) {

kumar303 · 2016-10-11T21:26:54Z

src/cmd/sign.js

+    }
+
+    try {
+      resolve(JSON.parse(oldUpdateManifest));


Why do you need resolve() here? I think if you declare fetchUpdateManifest as async then you can simply do return JSON.parse(oldUpdateManifest) on this line.

kumar303 · 2016-10-11T21:27:43Z

src/cmd/sign.js

+    } catch (e) {
+      throw new WebExtError(
+        `Unable to parse ${updateManifestFileName} file located at ` +
+        `${manifestData.applications.gecko.update_url}`);


This messages needs to include the original error message and try to avoid one character variables. I would suggest calling it catch(error)

kumar303 · 2016-10-11T21:31:16Z

src/cmd/sign.js

+  // note: `var == null` is equal to
+  // `(variable === undefined || variable === null)`
+  // see: https://stackoverflow.com/questions/2647867
+  if (manifestData == null ||


the top-level sign() function accepts the id parameter which gets a value when someone does web-ext sign --id=custom@example. This validation check looks like it is not honoring that id variable (for when it's not undefined).

I'm not sure what you mean here, the prevalidateUpdateManifestParams function doesn't check anything related to the id, it is left alone completely.
The right id value gets selected earlier on in the code

if (id && manifestId) { throw new WebExtError( `Cannot set custom ID ${id} because manifest.json ` + `declares ID ${manifestId}`); } if (manifestId) { id = manifestId; } if (!id && idFromSourceDir) { log.info( `Using previously auto-generated extension ID: ${idFromSourceDir}`); id = idFromSourceDir; } if (!id) { log.warn('No extension ID specified (it will be auto-generated)'); }

And this id later gets passed on to the createNewUpdateManifest function

let signingResult = await signAddon({...}); [...] let extensionID; if (signingResult.id) { extensionID = signingResult.id; } else { extensionID = id; } [...] await createNewUpdateManifest( extensionID, signingResult.downloadedFiles[0], artifactsDir, manifestData, updateLink, oldUpdateManifestData, );

I'm probably just not completely getting what you mean here.

yeah, sorry, I have no idea what I was talking about either. I guess ignore it for now. I will do another review when you've addressed the other change requests anyway.

kumar303 · 2016-10-11T21:45:47Z

src/cmd/sign.js

+      'as a substitute for the XPI file');
+  }
+
+  return true;


this return value doesn't look like it's used by anything

kumar303 · 2016-10-11T21:49:23Z

src/cmd/sign.js

+  return fs.writeFile(
+    path.join(artifactsDir, updateManifestFileName),
+    newUpdateManifest,
+    function(error) {


you can't use the error callback here since you are returning a promise -- i.e. the caller needs be notified about the error too. You can use chaining instead:

return fs.writeFile(...) .catch((error) => { throw new WebExtError( `Was unable to write updated ${addonName}: ${error}`); });

kumar303 · 2016-10-11T21:51:02Z

src/program.js

+        'update-link': {
+          describe:
+            'A URL to where your XPI files will be hosted. ' +
+            'ex: https://example.com/plugins/{xpiFileName} ' +


New sentences should start with a capital letter. I think it would also be helpful to spell out Example:

kumar303 · 2016-10-11T21:51:55Z

src/program.js

+            'ex: https://example.com/plugins/{xpiFileName} ' +
+            'if you select this option, an updateManifest ' +
+            'file will be generated ' +
+            'in the web-ext-artifacts folder',


Say in the --artifacts-dir folder instead since this value is customizable.

kumar303 · 2016-10-11T21:54:11Z

src/util/net.js

+ * Returns a promise resolving to a tuple of [body, statusCode]
+ * Will throw an error on connection errors
+ */
+export function httpFetchFile(url: string): Promise<[string, number]> {


This code would be easier to write (and easier to test) if you use the request module. I don't mind adding it as a dependency.

kumar303 · 2016-11-29T21:47:56Z

Hi @Azeirah. There's no rush on this but I just wanted to check in and see if you had any questions? Thanks for your continued work on it -- I think this will be a useful feature!

Azeirah · 2016-12-05T18:22:11Z

Hey @kumar303, I ran into some walls while developing this feature. I came here expecting that it would be a breeze to implement the whole thing, and that was partially true, the feature wasn't too hard to implement. Everything else was a bit overwhelming for me, complying to fairly strict coding standards (flux), learning several new ES6 features and learning how to write tests. Now that some time has passed I'll try to tackle the issue again.

The feature itself would be very useful for me as well, so I will continue work on this. I'll let you know if I run into any troubles.

kumar303 · 2016-12-07T07:28:40Z

I understand completely. Each patch takes a bit more time on this project, especially in writing the tests. We require automated tests to make sure we don't regress the features accidentally. Let me know if you need any help.

kumar303 · 2017-01-03T19:28:17Z

Hi @Azeirah . Would it help to try landing a smaller patch for this feature? It still seems like a nice feature to have. Let me know if you are stuck on anything.

Azeirah · 2017-01-20T17:36:47Z

@kumar303 I've picked it up again, just finished doing my exams at school so I have some time again. Apologies for the little communication

…s and return value

…pendency to perform http-requests

Azeirah · 2017-01-24T21:12:03Z

I've pushed the changes where I fix the issues you pointed out.

kumar303 · 2017-01-24T21:54:23Z

Thanks! Can you merge in master and resolve the conflicts? That will make it easier for me to review and test.

shubheksha · 2017-06-29T17:40:59Z

Hey @Azeirah, would it be possible to pick this up any time soon again?

rpl · 2022-06-06T17:09:22Z

Closing as "state: do not merge", the issue is still open and technically not covered yet by web-ext itself, unfortunately this PR has been branched out quite some time ago and it is unlikely to be resumed from this old branch.

martijn added 2 commits October 3, 2016 17:35

Added net utility file

bd7ee32

Added automatic generation of updateManifest with --update-link flag

1d27c29

muffinresearch added the state: pull request ready label Oct 3, 2016

Azeirah commented Oct 3, 2016

View reviewed changes

kumar303 reviewed Oct 4, 2016

View reviewed changes

refactor: split web-ext updateManifest function up into three functio…

3a63b13

…ns for testability

martijn added 3 commits October 11, 2016 01:10

Fix: Existing tests for sign won't fail anymore because of added func…

24a01ee

…tionality

Refactor: Fix flow errors

47bd699

Refactor: Comply with eslint

3efb095

kumar303 suggested changes Oct 11, 2016

View reviewed changes

martijn added 3 commits January 20, 2017 18:38

Style: Implemented changes as requested by @kumar303

9f9ddc5

Style: Added type declarations for createNewUpdateManifest's argument…

aa9706b

…s and return value

Docs: Improved documentation on sign's --update-link parameter

88e5385

kumar303 mentioned this pull request Jan 20, 2017

chore: turn on no-shadow lint rule #768

Merged

kumar303 self-assigned this Jan 23, 2017

Refactor: Remove util file and add 'request' and 'request-promise' de…

b41b959

…pendency to perform http-requests

kumar303 changed the title ~~Add feature to automatically generate updateManifest files from previous updateManifests~~ feat: Automatically generate updateManifest files from previous updateManifests Feb 2, 2017

kumar303 removed their assignment Feb 16, 2017

MarZab mentioned this pull request Aug 21, 2018

automatically generate update manifest for self-hosted updates #507

Open

rpl added state: do not merge and removed state: pull request ready labels Jun 6, 2022

rpl closed this Jun 6, 2022

feat: Automatically generate updateManifest files from previous updateManifests #552

feat: Automatically generate updateManifest files from previous updateManifests #552

Uh oh!

Conversation

Azeirah commented Oct 3, 2016

Usage and testing:

The command that I used

What I did

What still needs to be done:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumar303 commented Oct 4, 2016

Uh oh!

Azeirah commented Oct 6, 2016

Uh oh!

Azeirah commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumar303 commented Nov 29, 2016

Uh oh!

Azeirah commented Dec 5, 2016

Uh oh!

kumar303 commented Dec 7, 2016

Uh oh!

kumar303 commented Jan 3, 2017

Uh oh!

Azeirah commented Jan 20, 2017

Uh oh!

Azeirah commented Jan 24, 2017

Uh oh!

kumar303 commented Jan 24, 2017

Uh oh!

shubheksha commented Jun 29, 2017

Uh oh!

rpl commented Jun 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Azeirah commented Oct 10, 2016 •

edited

Loading