Skip to content

feat: Added release channel as optional sign parameter#1250

Closed
mi-g wants to merge 2 commits into
mozilla:masterfrom
mi-g:master
Closed

feat: Added release channel as optional sign parameter#1250
mi-g wants to merge 2 commits into
mozilla:masterfrom
mi-g:master

Conversation

@mi-g
Copy link
Copy Markdown

@mi-g mi-g commented Feb 12, 2018

Fixes #877

Added release channel as optional sign parameter.

To be effective, this requires a version of the sign-addon module patched with PR #249 or #250.

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 12, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 0892ee4 on mi-g:master into c505c06 on mozilla:master.

Copy link
Copy Markdown
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

This looks pretty good but it needs some test coverage so we don't accidentally break our integration with sign-addon.

I would suggest making a copy of this test and then remove all the assertions and add one more to check that channel gets passed through to signAddon().

@mi-g
Copy link
Copy Markdown
Author

mi-g commented Feb 12, 2018

I added the test for channel parameter passing to the signer. I hope this is ok.

@stoically
Copy link
Copy Markdown
Member

It might be worth to consider merging this in the light of the soon to be deactivated beta channel on AMO, since it helps with getting signed xpis for the selfhosted channel on the commandline without having to use the AMO UI.

@rpl rpl changed the title Added release channel as optional sign parameter feat: Added release channel as optional sign parameter Mar 23, 2018
@sneakypete81
Copy link
Copy Markdown
Contributor

Is there anything preventing this from being merged @kumar303?
Merging this would make hosting beta versions much more pleasant.

All I can think of is to reduce the number test assertions to only those that we care about - something more like this test.

Copy link
Copy Markdown
Contributor

@kumar303 kumar303 left a comment

Choose a reason for hiding this comment

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

@sneakypete81 yeah, the test could use some clean-up. Other than that, we would need an issue on file so we can give it the needs: docs label.

My web-ext domain knowledge is a bit rusty these days, so @rpl should probably take a look, too.

timeout: stubs.signingConfig.timeout,
version: stubs.preValidatedManifest.version,
xpiPath: stubs.buildResult.extensionPath,
channel: 'unlisted',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the only important part so you can remove all other parameter assertions.

@sneakypete81
Copy link
Copy Markdown
Contributor

Other than that, we would need an issue on file so we can give it the needs: docs label.

How about #877?

@kumar303
Copy link
Copy Markdown
Contributor

Yep, that looks like the same issue. I updated the description so it will close that one. Thanks.

@sneakypete81
Copy link
Copy Markdown
Contributor

@mi-g - I hope you don't mind that I applied @kumar303's review comment in PR #1400. I'm keen to get this feature merged and released.

@mi-g
Copy link
Copy Markdown
Author

mi-g commented Nov 25, 2018

@mi-g - I hope you don't mind that I applied @kumar303's review comment in PR #1400. I'm keen to get this feature merged and released.

No problem.

@kumar303
Copy link
Copy Markdown
Contributor

This patch was amended and merged via #1400

@kumar303 kumar303 closed this Feb 12, 2019
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.

5 participants