-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/ -----------------------------------------------------------
Review request for Review Board.
Description
-------
Automatic SSH key association should only be performed if the key has not
already been associated. HostingServices that support SSH key association
now have the capability to report if a given key has been associated with a
given repository. Currently, only the GitHub HostingService supports SSH
key association.
Added unit test for code that checks if a key has been associated. Made slight adjustment to key association test to ensure tests pass (given that a helper method for SSH key association
API calls was added). Both unit tests pass.
Tested manually with an existing GitHub repository by:
1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed key was associated.
2) adding SSH key to list of user keys. Check confirmed key was associated.
3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4513 -----------------------------------------------------------
It'd be nice to change the code to, at the best case, only check one batch of keys. So, get one set, check them. If not found, get the other set, check them.
Not sure which set to check first. Guess if we're eventually going to upload deploy keys, we should do that.
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 8, 2012, 12:54 a.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/ -----------------------------------------------------------
(Updated Nov. 8, 2012, 1:40 a.m.)
Review request for Review Board.
Changes
-------
Addressed most issues in Christian's review of r1
Description
-------
Automatic SSH key association should only be performed if the key has not
already been associated. HostingServices that support SSH key association
now have the capability to report if a given key has been associated with a
given repository. Currently, only the GitHub HostingService supports SSH
key association.
Added unit test for code that checks if a key has been associated. Made slight adjustment to key association test to ensure tests pass (given that a helper method for SSH key association
API calls was added). Both unit tests pass.
Tested manually with an existing GitHub repository by:
1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed key was associated.
2) adding SSH key to list of user keys. Check confirmed key was associated.
3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
4) ensured associate_ssh_key still works as normal for GitHub.
> > It'd be nice to change the code to, at the best case, only check one batch of keys. So, get one set, check them. If not found, get the other set, check them.
> > Not sure which set to check first. Guess if we're eventually going to upload deploy keys, we should do that.
Agreed. Deploy keys will probably be more likely, especially if this feature gets used.
> > Why are we factoring this out? Will more things be calling this?
No -- only is_ssh_key_associated and associate_ssh_key use it. It might be overkill, but it cleaned up the code a little. It's no trouble to scratch it, so just let me know.
- Karl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4513 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 8, 2012, 1:40 a.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4533 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 8, 2012, 1:40 a.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
> > Why did you change the single backticks to be double backticks?
Christian mentioned the change as part of his feedback on r1. I assumed it was because Sphinx uses reStructuredText for formatting, and I had written the docstring with my brain running in Markdown.
- Karl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4533 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 8, 2012, 1:40 a.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/ -----------------------------------------------------------
(Updated Nov. 12, 2012, 11:31 p.m.)
Review request for Review Board.
Changes
-------
Fix some alignment issues (code style).
Description
-------
Automatic SSH key association should only be performed if the key has not
already been associated. HostingServices that support SSH key association
now have the capability to report if a given key has been associated with a
given repository. Currently, only the GitHub HostingService supports SSH
key association.
Added unit test for code that checks if a key has been associated. Made slight adjustment to key association test to ensure tests pass (given that a helper method for SSH key association
API calls was added). Both unit tests pass.
Tested manually with an existing GitHub repository by:
1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed key was associated.
2) adding SSH key to list of user keys. Check confirmed key was associated.
3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
4) ensured associate_ssh_key still works as normal for GitHub.
> > Why did you change the single backticks to be double backticks?
> Karl Leuschen wrote:
> Christian mentioned the change as part of his feedback on r1. I assumed it was because Sphinx uses reStructuredText for formatting, and I had written the docstring with my brain running in Markdown.
Cool, just wondering.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4533 -----------------------------------------------------------
On Nov. 12, 2012, 11:31 p.m., Karl Leuschen wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 12, 2012, 11:31 p.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/ -----------------------------------------------------------
(Updated Nov. 16, 2012, 3:53 a.m.)
Review request for Review Board.
Changes
-------
Address issues in review of r3.
Description
-------
Automatic SSH key association should only be performed if the key has not
already been associated. HostingServices that support SSH key association
now have the capability to report if a given key has been associated with a
given repository. Currently, only the GitHub HostingService supports SSH
key association.
Added unit test for code that checks if a key has been associated. Made slight adjustment to key association test to ensure tests pass (given that a helper method for SSH key association
API calls was added). Both unit tests pass.
Tested manually with an existing GitHub repository by:
1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed key was associated.
2) adding SSH key to list of user keys. Check confirmed key was associated.
3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4544 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 12, 2012, 11:31 p.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4578 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 16, 2012, 3:53 a.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
Most of the issues you pointed out are on lines this patch doesn't modify, so I'll do a pass on the hostingsvcs app in another review request to clean up the PEP8 violations. Thanks for the heads up!
> > Continuation line indentation is not a multiple of four
This is shows up occasionally in the RB codebase -- this particular alignment is (slightly) more readable and trumps the PEP8 complaint in this case.
- Karl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4578 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 16, 2012, 3:53 a.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4608 -----------------------------------------------------------
I think after this one last set of changes, which are tiny, we'll be ready to ship it :D
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 16, 2012, 3:53 a.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/ -----------------------------------------------------------
(Updated Nov. 23, 2012, 4:22 p.m.)
Review request for Review Board.
Changes
-------
Fix style issues in r4 and test.
Description
-------
Automatic SSH key association should only be performed if the key has not
already been associated. HostingServices that support SSH key association
now have the capability to report if a given key has been associated with a
given repository. Currently, only the GitHub HostingService supports SSH
key association.
Added unit test for code that checks if a key has been associated. Made slight adjustment to key association test to ensure tests pass (given that a helper method for SSH key association
API calls was added). Both unit tests pass.
Tested manually with an existing GitHub repository by:
1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed key was associated.
2) adding SSH key to list of user keys. Check confirmed key was associated.
3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
4) ensured associate_ssh_key still works as normal for GitHub.
> > for url in (url_deploy_keys, url_user_keys):
> > keys_rsp = self._key_association_api_call(...)
> > blah blah
> > if formatted_key in ...
> > return True
> > Also, I'd prefer that "url" and "rsp" be suffixes, not prefixes.
Good tip -- thanks!
- Karl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4608 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 23, 2012, 4:22 p.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.reviewboard.org/r/3478/#review4621 -----------------------------------------------------------
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.reviewboard.org/r/3478/ > -----------------------------------------------------------
> (Updated Nov. 23, 2012, 4:22 p.m.)
> Review request for Review Board.
> Description
> -------
> Automatic SSH key association should only be performed if the key has not
> already been associated. HostingServices that support SSH key association
> now have the capability to report if a given key has been associated with a
> given repository. Currently, only the GitHub HostingService supports SSH
> key association.
> Added unit test for code that checks if a key has been associated. Made slight adjustment to > key association test to ensure tests pass (given that a helper method for SSH key association
> API calls was added). Both unit tests pass.
> Tested manually with an existing GitHub repository by:
> 1) adding RB server's (dev server's) SSH key to list of repository deploy keys. Check confirmed > key was associated.
> 2) adding SSH key to list of user keys. Check confirmed key was associated.
> 3) ensuring key NOT associated with repository. Check confirmed key NOT associated.
> 4) ensured associate_ssh_key still works as normal for GitHub.
