-
Notifications
You must be signed in to change notification settings - Fork 341
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
Export submit-addon via package.json #3250
base: master
Are you sure you want to change the base?
Conversation
e9c64c4
to
5be4f46
Compare
Test failure is blocking sec issues, I didn't add any deps so I don't think I can fix this. Note that locally the test I added runs as "pending", both with and without my changes. Might be something up with that one? |
The npm audit issues are pre-existing. I think that @rpl or @willdurand was going to address that. |
yes, I was the one that was meant to followup on that. The transitive dependency that was making us hitting the npm audit failure is part of sinon dependencies and despite the fact that the previous PR that was updating sinon to 18.0.1 was still failing (which was what I was going to take a look into), the new PR for sinon 19.0.0 (#3251) did actually pass the npm audit CI job step and so I proceeded with merging it. @kewisch would you mind to rebase this PR to confirm that the CI job failure is gone as expected? |
5be4f46
to
564fcd8
Compare
Looks like we're good :) |
We discussed about this in the triage meeting and we agreed to move forward with merging this PR but also update the README.md file to clarify what |
564fcd8
to
b51b896
Compare
I added a note to the readme, does that look good? |
In comment #3250 (comment) I was referring to expanding/rewording the existing Line 126 in cde10a8
Nonetheless, I think the note you added may also be worth keeping (because the sign command entrypoint is more likely to stay stable then the internal signAddon module). In the meantime, I'm going to approving and merging this version, we can followup to tweak that other note in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kewisch I've added one more review comment related to a just small nit/thought that I wanted to mention you explicitly before proceeding with merging this PR.
xpiPath: pathToExtension, | ||
savedUploadUuidPath: '.amo-upload-uuid', | ||
channel: 'unlisted', | ||
userAgentString: 'web-ext/0.2', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kewisch I think it may be worth to tweak this example to suggest using a userAgentString that makes it clear which web-ext version they are using and maybe include a custom part to highlight this was sent by using signAddon internal method programmatically
Fixes #3241