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
Added support for alternate SSH ports in AsusWRT (#4832) #6109
Conversation
@swbradshaw, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @linjef and @SEJeff to be potential reviewers. |
Hello @swbradshaw, When attempting to inspect the commits of your pull request for CLA signature status among all authors we encountered commit(s) which were not linked to a GitHub account, thus not allowing us to determine their status(es). The commits that are missing a linked GitHub account are the following:
Unfortunately, we are unable to accept this pull request until this situation is corrected. Here are your options:
We apologize for this inconvenience, especially since it usually bites new contributors to Home Assistant. We hope you understand the need for us to protect ourselves and the great community we all have built legally. The best thing to come out of this is that you only need to fix this once and it benefits the entire Home Assistant and GitHub community. Thanks, I look forward to checking this PR again soon! ❤️ |
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.
Even though it is super simple, best seems to be a single code path for the ssh args.
|
||
if self.protocol == 'ssh': | ||
|
||
if self.port != DEFAULT_SSH_PORT: | ||
self.ssh_args['port'] = self.port |
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.
This is entirely fine, but my take would be to simply always be explicit about the port. That way the code path that you use when specifying an alternate port is always used.
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.
I agree. I think when I made this change a couple months ago, I did so because the asus_wrt tests were really picky about the port, so I was trying to keep the port 22 when nothing was specified. I was trying (unsuccessfully) to get the tests to pass. I have since figured out the tests, so I'll make this change.
Hi @swbradshaw, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
|
||
if self.protocol == 'ssh': | ||
|
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.
blank line contains whitespace
Hi @swbradshaw, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
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.
Looks good to me (from a python standpoint), but I'm just getting back into the swing of contributing to HASS, and would prefer someone who's been a bit more active than me to merge this.
@swbradshaw mind signing the CLA? It isn't a (c) assignment or anything nasty like that.
Also you'll need to update the tests for the new invocation to include port=22
I believe.
@SEJeff The CLA was already signed. It looks like the Travis builds are broken after I made the change you suggested. :-( This is the part I got stuck on last time. Give me a couple days and I'll see if I can correct these. |
@@ -112,12 +115,16 @@ def __init__(self, config): | |||
self.ssh_key = config.get('ssh_key', config.get('pub_key', '')) | |||
self.protocol = config[CONF_PROTOCOL] | |||
self.mode = config[CONF_MODE] | |||
self.port = config.get(CONF_PORT, 22) |
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.
use only config[CONF_PORT]
you have set default on vol.
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.
@pvizeli Thanks for the feedback. Commited this change.
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.
These changes were completed in latest commit
My Asus router doesn't run SSH on port 22. This allows Home Assistant to work now by exposing this the SSH port as a parameter.
Related issue (if applicable): fixes #4832
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#1721
Example entry for
configuration.yaml
(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass