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

feat: Added optional `channel` parameter #249

Merged
merged 1 commit into from Feb 14, 2018

Conversation

Projects
None yet
3 participants
@welwood08
Copy link
Contributor

welwood08 commented Feb 10, 2018

Fixes #248

I'm not sure where I should add unit tests if any, would appreciate pointers and I can update this PR. I have tested the changes using my addon: I can specify unlisted channel when the previously used channel was listed and the result is unlisted ready for self-hosting.

@welwood08 welwood08 force-pushed the welwood08:issue-248 branch from 941eb42 to 5bdfea7 Feb 10, 2018

@kumar303
Copy link
Member

kumar303 left a comment

Hi. Thanks for starting a patch for this. Other folks have been asking for this feature.

Every code change must have test coverage. Read through the developer docs for info on how to run the tests: https://github.com/mozilla/sign-addon#development

@welwood08 welwood08 force-pushed the welwood08:issue-248 branch 2 times, most recently from 8684e0f to a113f58 Feb 12, 2018

@welwood08

This comment has been minimized.

Copy link
Contributor Author

welwood08 commented Feb 12, 2018

I think I found all the right places for tests to be added, let me know if anything else is missing.

@kumar303
Copy link
Member

kumar303 left a comment

Thanks for the quick follow-up. This approach looks good, I just have some requests for clean-up. It's almost there.

@@ -14,6 +14,9 @@ export default function signAddon(
id,
// The add-on version number for AMO.
version,
// The release channel on AMO (listed or unlisted).
// Defaults to most recently used channel.
channel=null,

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 12, 2018

Member

For consistency, this should default to undefined, meaning you don't need to set a default value for it.

This comment has been minimized.

Copy link
@welwood08

welwood08 Feb 12, 2018

Author Contributor

Had another look at this and I think I should move it below // Optional arguments: and remove the =null part to match how the other optional arguments are defined.

* @return {Promise} signingResult with keys:
* - success: boolean
* - downloadedFiles: Array of file objects
* - id: string identifier for the signed add-on
*/
sign({guid, version, xpiPath}) {
sign({guid, version, channel=null, xpiPath}) {

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 12, 2018

Member

This can just be channel, without a default value

@@ -93,6 +94,9 @@ export class Client {
// PUT to a specific URL for this add-on + version.
addonUrl += encodeURIComponent(guid) +
"/versions/" + encodeURIComponent(version) + "/";
if (channel) {
formData.channel = channel;
}
} else {

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 12, 2018

Member

The else block handles posting a new version for a new add-on. In that case, it might be confusing to the caller that channel will be ignored. I think this should log a message but not throw an error.

if (guid) {
  // ...
} else {
  if (channel) {
    this.logger.warn(
      `Posting a version for a new add-on on a specific channel is not supported. The version will be unlisted.'
    );
  }
}

The channel parameter will be implemented soon for POSTs but it will require changes to sign-addon because more data will need to be submitted.

@@ -46,6 +46,9 @@ signAddon(
// WebExtensions do not require an ID.
// See the notes below about dealing with IDs.
id: 'your-addon-id@somewhere',
// The release channel (listed or unlisted).

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 12, 2018

Member

This should also say: This parameter only takes effect when uploading a version for an existing add-on.

});
});

it("lets you sign an unlisted add-on", function() {

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 12, 2018

Member

This can be: "it lets you sign a version on a specific channel"

I don't think you need two separate tests for the listed and unlisted values -- one test to cover passing the value through to the API will be enough.

});
});

it("lets you sign a listed add-on", function() {

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 12, 2018

Member

Since all we're doing is passing a string value through to the API, I don't think you need this second test. It can be deleted.

expect(putCall.name).to.be.equal("put");

var partialUrl = "/addons/" + conf.guid + "/versions/" + conf.version;
expect(putCall.conf.url).to.include(partialUrl);

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 12, 2018

Member

All of these assertions are already covered by other tests. You can remove all of them except for this one:

expect(putCall.conf.formData.channel).to.be.equal("unlisted");

@welwood08 welwood08 force-pushed the welwood08:issue-248 branch 2 times, most recently from 50fbf01 to 652a77c Feb 12, 2018

@welwood08

This comment has been minimized.

Copy link
Contributor Author

welwood08 commented Feb 12, 2018

I believe I've addressed all your comments, please take another look (I don't know if there are notifications for new code or passing tests).

@welwood08 welwood08 changed the title Allow specifying release channel when signing feat: Added optional `channel` parameter Feb 12, 2018

@welwood08 welwood08 force-pushed the welwood08:issue-248 branch from 652a77c to 340eb8c Feb 12, 2018

@kumar303
Copy link
Member

kumar303 left a comment

Thanks. This just needs a bit more test coverage otherwise it looks good. I tested it with web-ext just to see that the existing signing process still works -- everything went smooth.

} else {
// POST to a generic URL to create a new add-on.
this.debug("Signing add-on without an ID");
method = "post";
formData.version = version;
if (channel) {
this.logger.warn(

This comment has been minimized.

Copy link
@kumar303

kumar303 Feb 13, 2018

Member

This line of code needs some test coverage. You don't need to make an assertion about the log message, the code just needs to be executed. This will help us make sure executing the log statement doesn't throw an error after some refactoring (or a change to the logger or whatever).

Here is a patch to check that you have test coverage:

diff --git a/src/amo-client.js b/src/amo-client.js
index fe5c116..8f79545 100644
--- a/src/amo-client.js
+++ b/src/amo-client.js
@@ -103,6 +103,7 @@ export class Client {
       method = "post";
       formData.version = version;
       if (channel) {
+        throw new Error("This should cause a test failure");
         this.logger.warn(
           "Specifying a channel for a new add-on is unsupported. " +
           "New add-ons are always in the unlisted channel."

This comment has been minimized.

Copy link
@welwood08

welwood08 Feb 13, 2018

Author Contributor

Test added.

@welwood08 welwood08 force-pushed the welwood08:issue-248 branch from 340eb8c to dc5568a Feb 13, 2018

@kumar303
Copy link
Member

kumar303 left a comment

This is a great patch! Thanks for all the fix-ups.

@kumar303 kumar303 merged commit fb5bf84 into mozilla:master Feb 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@caitmuenster

This comment has been minimized.

Copy link

caitmuenster commented Feb 14, 2018

Thanks so much, @welwood08! Your contribution has been added to our recognition wiki.

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Welcome onboard! I look forward to seeing you around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.