Skip to content

write id to file upon successful submission #2511

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

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

eviljeff
Copy link
Member

fixes #2491 - and could address #2490 (at least in part) by clearly stating the addon id after it's available from AMO.

@eviljeff eviljeff requested review from rpl and willdurand and removed request for rpl September 12, 2022 14:14
Comment on lines +298 to +305
log.info(`Generated extension ID: ${addonId}.`);
log.info('You must add the following to your manifest:');
log.info(`"browser_specific_settings": {"gecko": "${addonId}"}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move saveIdToSourceDir() somewhere else (could be in this file even), then those logs could also be moved to saveIdToSourceDir(). If there is a chance that this message does not apply, we could update the content a bit (e.g. from "You must add [...]" to "Make sure you have [...] in your manifest:").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id is never needed for the signAddon route, while AMO doesn't allow MV3 submissions. (If we're specifically adding support for MV3 I'd prefer that in a separate PR).

We only call the function for id-less submissions for submit-addon, so it's always needed.

@eviljeff eviljeff requested a review from willdurand October 11, 2022 12:17
@eviljeff eviljeff force-pushed the 2491-save-generated-id-earlier branch from 8f64065 to 7fe826f Compare October 13, 2022 16:35
@eviljeff
Copy link
Member Author

had to squash and rebase to address the conflicts from #2489

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just a small nit (also not a super big deal, it is just a function name mentioned in an inline comment that may need to be updated and so I'm also approving this version in the meantime).

src/cmd/sign.js Outdated
@@ -185,28 +193,26 @@ export default function sign(
throw new Error('The extension could not be signed');
}
result = { id: newId, downloadedFiles };
// All information about the downloaded files would have already been
// logged by signAddon(). submitAddon() calls saveIdToSourceDir itself.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, saveIdToSourceDir => saveIdToFile in this inline comment?

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

Successfully merging this pull request may close these issues.

save the addon id earlier, after a successfull submission?
3 participants