remote: add support for ssh config #1965
Conversation
dvc/remote/ssh/__init__.py
Outdated
|
|
||
| user_config_file = os.path.expanduser("~/.ssh/config") | ||
| user_ssh_config = dict() | ||
| if self.host: |
There was a problem hiding this comment.
Could you please elaborate why if self.host is needed?
There was a problem hiding this comment.
@efiop it's fix only for tests :(
dependency test
https://github.com/iterative/dvc/blob/master/tests/func/test_dependency.py#L20
create remotes with empty configs
here the actual place where empty remote config created
https://github.com/iterative/dvc/blob/master/dvc/output/base.py#L74
also we have weird place
https://github.com/iterative/dvc/blob/master/dvc/remote/ssh/__init__.py#L37
self.url = config.get(Config.SECTION_REMOTE_URL, "ssh://")obviously ssh will not work with such default url and it should be error.
I think it also fix for dependency test, but right now I don't know how make it beautiful. But I will research it as a separate task.
dvc/remote/ssh/__init__.py
Outdated
| user_config_file = os.path.expanduser("~/.ssh/config") | ||
| user_ssh_config = dict() | ||
| if self.host: | ||
| if os.path.exists(user_config_file): |
There was a problem hiding this comment.
Let's put all of this config parsing logic into a separate method/function and use it.
|
Also, could you please submit a fix for https://dvc.org/doc/commands-reference/remote-modify for SSH? |
Amend to docs, created PR dvc.org#290 |
There was a problem hiding this comment.
Thanks! Looks great! 🙂
One more thing, would you be so kind to also add a test to tests/unit/remote/ssh/test_ssh.py to make sure that your patch works as expected? E.g. that user, port, hostname are read from ssh config properly and in correct order.
| parsed = urlparse(self.url) | ||
| self.host = parsed.hostname | ||
|
|
||
| user_ssh_config = self._load_user_ssh_config(self.host) |
There was a problem hiding this comment.
Btw, if we are looking up Host in the ssh config, shouldn't we use HostName after that?
There was a problem hiding this comment.
You are right, we should use HostName from ssh config if it exists.
add tests and also add using |
|
@khamutov Thanks! :) Just a heads up: your tests are failing. |
dvc/remote/ssh/__init__.py
Outdated
|
|
||
| @staticmethod | ||
| def _load_user_ssh_config(hostname): | ||
| user_config_file = os.path.expanduser("~/.ssh/config") |
There was a problem hiding this comment.
iterative/dvc.org#290 (comment) Let's use os.path.join("~", ".ssh", "config") instead, since on windows it won't resolve it properly.
There was a problem hiding this comment.
accepted, will fix it after test passed on Travis
tests/unit/remote/ssh/test_ssh.py
Outdated
| """ | ||
|
|
||
| # compat version for mock library version 2 | ||
| # mock 2.0.0 can't iterate in mock_open |
There was a problem hiding this comment.
You could just adjust mock version requirements in tests/requirement.txt up to the needed version, we just didn't update it for a long time 🙂
There was a problem hiding this comment.
Done. I misinterpreted mock documentation "Version 2.0.0 is the last version compatible with Python 2.6." But we have min version 2.7 :)
efiop
left a comment
efiop
left a comment
There was a problem hiding this comment.
Amazing! Thank you so much! 🎉
|
Hi @khamutov ! Thanks again for your contribution! We would love to have a chance to meet you :) Would you be willing to have a hangouts call with us some time? Btw, I see you are from Moscow, our OG dvc creator @dmpetrov is going to present at Data Fest this weekend, please feel free to come say hi if you are attending too 🙂 Thanks, |
|
@khamutov I’d love to meet. Please let me know if you are in DataFest today. I’m giving a talk about open source tools for ml processes (including DVC). Also I’m staying in Moscow till Tuesday. |
3 participants
Have you followed the guidelines in our
Contributing document?
Does your PR affect documented changes or does it add new functionality
that should be documented? If yes, have you created a PR for
dvc.org documenting it or at
least opened an issue for it? If so, please add a link to it. Created PR dvc.org#290
Fixes #1613
Add parsing user ssh config for username and port. However settings from remote url has the priority over ssh config.