Repository: iterative/dvc
Pull request: ssh credential prompt before progress
- fixes #2653

Problem statement:
"dvc pull" shows progress bar too early 
**Information about setup**

DVC version: 0.63.4
Method of installation: pip
Platform: Ubuntu Linux 16.04

**Description:**
I access my dvc remote via ssh. Whenever I execute `dvc pull` command dvc asks for passphrase for my RSA key and immediately shows progress bar before I've entered anything from my keyboard:

```
Enter a private key passphrase or a password for host 'ipaddress' port 'port' user 'dvcrepo':
  0%|          |Querying cache in ssh://dvcrepo@ipaddress:port/path/to/dvc                                                                                                                                                  0/1 [00:00<?,     ?file/s]
```

Such format is confusing: first time I've executed `dvc pull` I was wondering for several minutes why cache quering stuck on 0% before I realized that dvc expects me to enter passphrase.

Progress bar should be shown **after** I've entered passphrase.

Hints from issue discussion:
@casperdcl could you take a look please?
ah, the bar might actually be showing before the prompt, but it's just drawn below all text (including the prompt).

Commit message: ssh credential prompt before progress

Patch to review:
diff --git a/dvc/remote/ssh/__init__.py b/dvc/remote/ssh/__init__.py
index 75a2a6f980..168d1eff3b 100644
--- a/dvc/remote/ssh/__init__.py
+++ b/dvc/remote/ssh/__init__.py
@@ -115,9 +115,7 @@ def _try_get_ssh_config_keyfile(user_ssh_config):
             return identity_file[0]
         return None
 
-    def ssh(self, path_info):
-        host, user, port = path_info.host, path_info.user, path_info.port
-
+    def ensure_credentials(self, host, user, port):
         # NOTE: we use the same password regardless of the server :(
         if self.ask_password and self.password is None:
             with saved_passwords_lock:
@@ -133,6 +131,10 @@ def ssh(self, path_info):
                     )
                 self.password = password
 
+    def ssh(self, path_info):
+        host, user, port = path_info.host, path_info.user, path_info.port
+        self.ensure_credentials(host, user, port)
+
         from .connection import SSHConnection
 
         return get_connection(
@@ -306,6 +308,10 @@ def cache_exists(self, checksums, jobs=None, name=None):
         if not self.no_traverse:
             return list(set(checksums) & set(self.all()))
 
+        # possibly prompt for credentials before "Querying" progress output
+        path_info = self.checksum_to_path_info(checksums[0])
+        self.ensure_credentials(path_info.host, path_info.user, path_info.port)
+
         with Tqdm(
             desc="Querying "
             + ("cache in " + name if name else "remote cache"),


Code review comments:
diff_hunk: @@ -306,6 +308,10 @@ def cache_exists(self, checksums, jobs=None, name=None):
         if not self.no_traverse:
             return list(set(checksums) & set(self.all()))
 
+        # possibly prompt for credentials before "Querying" progress output
+        path_info = self.checksum_to_path_info(checksums[0])
+        self.ensure_credentials(path_info.host, path_info.user, path_info.port)
path: dvc/remote/ssh/__init__.py
text: @user1:
Hm, seems like we could simply do
```
with self.ssh(self.path_info):
    pass
```
And that way we won't need `ensure_credentials` at all.

@author:
what about https://github.com/iterative/dvc/blob/94cfb840a25292cf72e558242cd294023a01f17c/dvc/remote/ssh/__init__.py#L138-L149

which is unneeded for just ensuring credentials exist? Is it not worth worrying about?

@user1:
@author Mm, not sure I follow. What is concerning you about that piece of code?

@user1:
Aah, got it. You mean that we don't have to establish the connection to check that we have the password, right?

@user1:
You are right, I've missread the way `def ssh` got split.

@author:
right; though I missed `self.path_info`. I presume I should use that instead of the hacky `self.checksum_to_path_info(checksums[0])`? As the pre-existing comments say, it's unfortunate that per-path credentials aren't supported.

@user1:
Ok, then one more thing:
```
        path_info = self.checksum_to_path_info(checksums[0])
```
is not really needed. We can use self.path_info instead
```
self.ensure_credentials(self.path_info.host, self.path_info.user, self.path_info.port)
```
Though, I would probably change it to just accept path_info like so
```
self.ensure_credentials(self.path_info)
```

@author:
:P I answered first