Ensure SSH is given a valid port number.
Review Request #7619 — Created Sept. 8, 2015 and submitted
When an invalid port is given in a repository path, a cryptic error message was shown.
The change provides a better error message to the user when an invalid port is in the repository path.
Ran unit tests.
Manually verified that the error message was displayed upon replication.
Description | From | Last Updated |
---|---|---|
This doesn't need to be in the try does it? |
|
|
What happens if this doesn't pass a list. Also, there is trailing whitespace and this should end with a period. |
|
|
SSHError isn't really appropriate in this case because its an error in parsing, not with SSH itself. Can we add … |
|
|
Can this include the port number? Also, we use single quotes for strings. |
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
Col: 17 W503 line break before binary operator |
![]() |
|
This isn't going to work quite correctly, because whatever is passed to _() needs to be a constant. How about … |
|
|
Col: 17 E128 continuation line under-indented for visual indent |
![]() |
|
I'd call this SSHInvalidPortError, for consistency with the other classes. |
|
|
Blank line between these. |
|
|
For docstrings, you should use double quotes. """ |
|
|
You can combine these into one statement: You should also use super(...) for calling the parent constructor. The other classes … |
|
|
Make sure these are imported in alphabetical order. |
|
|
Blank line between these. We always have a blank line between statements and blocks, or blocks and blocks on the … |
|
|
"path" -> "port" |
|
|
Elsewhere in this file, instead of using super(class), we seem to directly call into SSHError.__init__. Do we know why that … |
|
|
You don't need this blank line, since the block is ending. |
|
- Bugs:
-
I appreciate the change, but we actually don't validate URLs in the form for a very good reason: Repository paths are not always URLs.
Perforce uses paths like myservername:1666.
CVS uses paths like :pserver:username@servername:/cvsroot/foo
Git uses names like git@domain:/path
Even when they are URLs, sometimes they're not fully-qualified domains (so https://myserver/, rather than https://myserver.mycompany.com/), which won't pass the validator.
Repository path validation would need to be done in each SCMTool instead, per their own validation rules.
Right now, we already have a
check_repository()
function thatSCMTool
subclasses can implement, which checks to make sure the URL is accessible. This is where validation logic should go. It should ensure that the path passed in matches expectations, and raise a suitable error (perhaps a newRepositoryPathValidationError
that subclassesSCMError
). The form and API code would need to know to handle this new error.
- Summary:
-
Add URLValidation to form.py to validate repo's pathAdd try/except block in ssh/utils.py to catch empty port
- Description:
-
~ Add URLValidation to form.py to validate repo's path
~ Added a try/except block to catch a Value error that occurs when ssh/utils.py check_host() gets an empty port string.
+ + The original bug, was caused by the user entering in an incorrect path, but I feel the error message should better reflect what was causing the actual error, which in this case was using int() on an empty string. I worded the error in a way that would hopefully cause the reader to double check their path even if they weren't intending to put in a port number (which was the case for bug3891).
+ + I was unsure if a new error in ssh/errors.py would be justified, so I didn't create one.
- Branch:
-
masterrelease-2.0.x
- Diff:
-
Revision 2 (+4 -1)
![reviewbot](https://reviews.reviewboard.org/static/ext/reviewbotext.extension.ReviewBotExtension/images/reviewbot.png)
-
Tool: PEP8 Style Checker Processed Files: reviewboard/ssh/utils.py Tool: Pyflakes Processed Files: reviewboard/ssh/utils.py
-
Your description and summary need some work. You may want to give this guide on writing good descriptions a read.
In general, the summary and description should provide information on why the change was necessary.
-
SSHError
isn't really appropriate in this case because its an error in parsing, not with SSH itself.Can we add a new error class somewhere that describes a parsing error?
- Summary:
-
Add try/except block in ssh/utils.py to catch empty portEnsure SSH is given a valid port number.
- Description:
-
~ Added a try/except block to catch a Value error that occurs when ssh/utils.py check_host() gets an empty port string.
~ When an invalid port is given in a repository path, a cryptic error message was shown.
~ The original bug, was caused by the user entering in an incorrect path, but I feel the error message should better reflect what was causing the actual error, which in this case was using int() on an empty string. I worded the error in a way that would hopefully cause the reader to double check their path even if they weren't intending to put in a port number (which was the case for bug3891).
~ The change provides a better error message to the user when an invalid port is in the repository path.
- - I was unsure if a new error in ssh/errors.py would be justified, so I didn't create one.
![reviewbot](https://reviews.reviewboard.org/static/ext/reviewbotext.extension.ReviewBotExtension/images/reviewbot.png)
-
Tool: PEP8 Style Checker Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py Tool: Pyflakes Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py
![reviewbot](https://reviews.reviewboard.org/static/ext/reviewbotext.extension.ReviewBotExtension/images/reviewbot.png)
-
Tool: PEP8 Style Checker Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py Tool: Pyflakes Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py
-
-
-
-
-
You can combine these into one statement:
You should also use
super(...)
for calling the parent constructor. The other classes don't do this, but only because they used to have to support versions of Python wheresuper()
couldn't be used with exceptions.End result should be like:
super(InvalidSSHPortError, self).__init__( self, _('......' '......') % port)
Note two things about this:
- When dealing with long parameters like this, we're doing one parameter per line.
- The
% var
goes on its own line, aligned with th thing it's formatting, when dealing with wrapped strings.
-
-
Blank line between these. We always have a blank line between statements and blocks, or blocks and blocks on the same level.
![reviewbot](https://reviews.reviewboard.org/static/ext/reviewbotext.extension.ReviewBotExtension/images/reviewbot.png)
-
Tool: Pyflakes Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py Tool: PEP8 Style Checker Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py
- Groups:
![reviewbot](https://reviews.reviewboard.org/static/ext/reviewbotext.extension.ReviewBotExtension/images/reviewbot.png)
-
Tool: Pyflakes Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py Tool: PEP8 Style Checker Processed Files: reviewboard/ssh/utils.py reviewboard/ssh/errors.py