search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

error on submitting a new addon as channel=listed via the api #7079

Closed Callek closed 3 years ago

Callek commented 4 years ago

For Firefox 71 beta's we added new locales (trs and tl) which had never had an entry in AMO before.

On submission they were properly signed but AMO took them as unisted, which broke releng's process of going in and manually uploading them so they can be on the site as listed.

Last time we added a new locale and thus a new langpack was bn in 68's beta cycle, where this worked then.

CC: @flodolo

diox commented 4 years ago

Are you sure you submitted them as listed? The API would not let you do that.

Callek commented 4 years ago

@diox Yes, I'm sure.

The process we used to follow for "not yet on AMO" langpacks, was:

  1. Automation creates language pack
  2. Automation submits language pack to AMO, citing 'listing' needed.
  3. AMO never signs it, waiting for the metadata we need to be filled out on the web side.
  4. Releng Goes to AMO web, uploads the just-generated langpack, and fills in metadata, as listed.
  5. Releng retriggers automation task which succeeds because it can now find that signed language pack and continues.

https://github.com/mozilla-releng/releasewarrior-2.0/blob/master/docs/addons/addons.md#how-to-handle-new-languages-for-release

One thing I realized that changed on my end in this timeframe was releng now signs langpacks via autograph before submission (under the thought that we didn't re-sign if submitted as a mozilla.com user, but turns out that was partially incorrect) -- anyway that was done over in https://bugzil.la/1566298

diox commented 4 years ago

We haven't changed the logic around this API for years. The docs mention this:

channel is only valid for new versions on existing add-ons. If the add-on is new then the version will be created as unlisted.

So this matches your original comment, but this is not new behavior at all (though I agree it's surprising that it does this silently).

The error in the procedure you describe in your follow-up comment happens for new listed versions of an existing purely unlisted add-on: in that case, the add-on exists, we try to obey the channel, but since it's missing metadata to have a listing on AMO, we raise a 400 error.

The code: https://github.com/mozilla/addons-server/blob/0a88d60c30a2c76563c8769382baf84161ca4590/src/olympia/signing/views.py#L206-L232

Callek commented 4 years ago

The interesting bit is bn on 68 never had an addon prior, but on submission as listed raised the 400 and didn't move on...

Now it submits as unlisted and succeeded even though it was expecting listed

diox commented 4 years ago

mmm. I'd need the log of all the API calls you made and the responses you got to debug this.

Callek commented 4 years ago

Firefox 71.0b3 -- trs and tl are in this log: https://taskcluster-artifacts.net/dAdvf5clQQ2lgbh0eCdcxw/0/public/logs/live_backing.log which is from https://tools.taskcluster.net/tasks/dAdvf5clQQ2lgbh0eCdcxw

So apparantly!?!? - Version 68.0buildid20190520131124 for bn was indeed unlisted while Version 68.0buildid20190521110747 wasn't.

There was a build1 and a build2 for bn

diox commented 4 years ago

So, everything I'm seeing here appears to be consistent:

Now obviously, consistent doesn't mean right :) I believe the right thing to do here would be to /try/ to obey the channel parameter if it's present at all times - if it's absent, defaulting to unlisted is fine, but I don't think there is a good reason behind ignoring it completely for new submissions.

It would mean the initial submission would always 400 if passing channel=listed, meaning you'd have to do your manual procedure to fix that, but it's probably better than completely disregarding the channel the developer intended to use.

eviljeff commented 4 years ago

It could error if the parameter was provided.

the channel parameter ... if it's absent, defaulting to unlisted is fine, but I don't think there is a good reason behind ignoring it completely for new submissions.

The channel parameter is invalid for new submissions as it's impossible to create a new addon with a listed version and the required metadata via the signing API. It probably shouldn't be silently ignored, but it's not that if it's absent the submission defaults to to unlisted, rather it's not used for new submissions as they can only be unlisted. The default is always whatever the previous version's channel was (which could be considered counter-intuitive but was needed for backwards compatibility at the time we merged listed and unlisted addons into addons with listed and unlisted versions)

*We hope for a future API that will allow all the things.

Callek commented 4 years ago

An alternative that would support releng's use case, if it makes more sense from a backend standpoint is one of:

The root of this problem is that releng can't ship a language pack until its built, the version code is baked into the shipping code's data as of push time (pushdate) and we need the language listed as of shipping time so that users can find it via in-app UX.

So not being able to 'fix'/account-for this is a problem.

diox commented 4 years ago

Doing either of those things would go against how versions work in AMO. It would have to be a specific extra development for releng, I don't think this is a good idea.

Erroring if trying to submit a listed initial submission is a lot easier and makes more sense for us (for as long as we don't have a way to accept initial listed submissions through the API).

escapewindow commented 4 years ago

Related: mozilla/addons#7908

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.