Skip to content
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

Make changes to handle the addon id existing from the previous submission #2490

Closed
eviljeff opened this issue Aug 26, 2022 · 9 comments
Closed

Comments

@eviljeff
Copy link
Member

Background:
AMO's submission API has three endpoints for creating a version from an xpi Upload:

  1. POST to /addons/addon/ to create a new add-on listing with the new version
  2. POST to /addons/addon/<amo-id|addon-id|slug>/versions/ to create a new version under an existing add-on listing. There isn't support for this endpoint in submit-addon.js code currently.
  3. PUT to /addons/addon/<addon-id>/ to either create a new add-on listing with the new version if the addon-id doesn't exist already; or to update an existing add-on listing and create a new version under that listing if the addon-id does already exist. This endpoint also enforces that the addon-id is defined in the manifest (so there is only one possible place to define the addon-id).

Current web-ext behaviour is to use endpoint 3. when an addon-id is defined in the manifest, provided via command-line arg, or exists in .web-extension-id; and to use endpoint 1. otherwise. After a successful add-on submission (including approval and download of the xpi) the addon-id from the API response is used to write the .web-extension-id file.

However, this behaviour is now flawed with the submission api if the developer didn't define the addon-id in the manifest - after a new add-on is created and the addon-id written to .web-extension-id, the next attempt to use web-ext sign will raise an error because there is an addon-id provided but it's not in the manifest file.

There are two options to fix this, as I see them:

  • add the addon-id to the manifest if not already there (in the build function? in getValidatedManifest?) if it's either provided via command line arg or in .web-extension-id. Requiring the addon-id in the manifest is a planned change anyway so web-ext will likely need to add support at some point regardless.
  • implement support for endpoint 2. and use that endpoint when there is .web-extension-id file instead. An addon-id in the manifest would continue to use endpoint 3. Note this still won't cover the scenario when a developer only provides the addon-id via the command line.
@eviljeff
Copy link
Member Author

@rpl and @willdurand for opinions.

@eviljeff
Copy link
Member Author

eviljeff commented Sep 9, 2022

  • add the addon-id to the manifest if not already there (in the build function? in getValidatedManifest?) if it's either provided via command line arg or in .web-extension-id. Requiring the addon-id in the manifest is a planned change anyway so web-ext will likely need to add support at some point regardless.

fwiw, I see this as the "correct" fix.

@willdurand
Copy link
Member

Requiring the addon-id in the manifest is a planned change anyway so web-ext will likely need to add support at some point regardless.

Yeah, that is true for MV3 and the linter already supports it.

As for updating the manifest...

When we submit an add-on for the first time, without the ID in the manifest, IIRC AMO will inject it automagically. Can we detect that in web-ext sign and output a message saying that the developer must update the manifest.json file (with the snippet of code that needs to be added)?

Still assuming that's something we can detect, could we show the same message if the user attempts to re-run web-ext sign?

@eviljeff
Copy link
Member Author

When we submit an add-on for the first time, without the ID in the manifest, IIRC AMO will inject it automagically. Can we detect that in web-ext sign and output a message saying that the developer must update the manifest.json file (with the snippet of code that needs to be added)?

We know that an xpi was submitted without an ID being provided so we could throw up a message once we get the API response with an ID back. (We could read it directly from the manifest, once signed, but that'd be more work overall).

Still assuming that's something we can detect, could we show the same message if the user attempts to re-run web-ext sign?

If there's an ID in web-extension.id it's from the previous submission and if it's not in the manifest we raise an error - that's a large part of what the issue is about. We could display an more extensive message with exact details of what to put in the manifest, sure. IMO it's not a great user experience though - I guess my assumption was wrong that web-ext would implement that behaviour on the back of the MV3 requirements. But it's an easier fix, admittedly.

@eviljeff
Copy link
Member Author

Some connection with #2491 here - because we can't really assume that the sign command will wait for a signed xpi and we only save the ID then.

@willdurand
Copy link
Member

I guess my assumption was wrong that web-ext would implement that behaviour on the back of the MV3 requirements.

MV3 manifests must have an extension ID.

@eviljeff
Copy link
Member Author

I guess my assumption was wrong that web-ext would implement that behaviour on the back of the MV3 requirements.

MV3 manifests must have an extension ID.

Yes, my assumption was that web-ext would nicely handle that particular jumping of the hoops for developers.

@willdurand
Copy link
Member

Ha, I see. Currently, it is only documented in the migration checklist: https://extensionworkshop.com/documentation/develop/manifest-v3-migration-guide/, I don't know about plans to automate this part.

@eviljeff
Copy link
Member Author

From the discussion above and offline, it seems like there is no immediate plan to add the id to the manifest automatically, and #2511 adds messaging that instructs the developer to add the ID to the manifest, so it doesn't seem there's anything else to do here right now. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants