Skip to content
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

create-sibling: Add support for local paths (v2) #4187

Merged
merged 17 commits into from Feb 25, 2020

Conversation

kyleam
Copy link
Collaborator

@kyleam kyleam commented Feb 25, 2020

This PR resurrects @yarikoptic's patch from gh-2615 that added local path support to create_sibling. The (rebased) commit from that PR sits as the first commit of this series. The final patch extends the existing create_sibling tests to go through the local path case. The patches in between are various tweaks and fixes.

When this was initially proposed in gh-2615, the main objection, raised by @mih, was that create_sibling should be reworked to not assume a local or remote shell. That is, put some abstraction layer over the calls to {SSHConnection,_RunnerAdapter}.__call__. On top of that, there was the idea that such an abstraction could lead to dissolving create_sibling_github.

The command abstractions being worked on in gh-4068 are bringing us closer to the first part (relying solely on methods rather than explicit shell commands). But in the meantime, this PR isn't making things any worse or more difficult going forward. The core logic added to CreateSibling boils down to a single condition about local vs remote, the one that decides whether to use SSHConnection or the local-path-based _RunnerAdapter.

As for the second point, with the addition of create_sibling_gitlab and create_sibling_ria, I think it's safe to conclude that the idea of unifying these variants should not prevent moving forward with the local path support in create_sibling. Assuming that such a union of create_sibling* variants is desirable, it isn't going to happen in the near future, and teaching create_sibling to handle local paths certainly isn't making the situation any worse than the recent create_sibling_* additions did.

Closes #1680.

yarikoptic and others added 17 commits Feb 25, 2020
Modified-by: Kyle Meyer <kyle@kyleam.com>
  This is rebased from the patch posted at dataladgh-2615.  No changes were
  made aside from dealing with conflicts.
If git-annex doesn't exist on the remote, we clumsily tell the user

    [ERROR  ] git-annex is missing. on the remote system

The period is coming from MissingExternalDependency.__str__(), and we
can't avoid it without reworking things there.  Instead rephrase the
message we give to work better after a period.
It's unlikely that a caller gets to the point of calling
create-sibling if git-annex isn't installed locally, but it's
possible.  And even if it weren't, we might as well make the code path
for SSH and local paths as close as possible.
With Python 3, copytree() allows the caller to specify an alternative
copy function that will be used instead of the attribute-preserving
copy2().
All else being equal, this reader finds `if x ... else ...` easier to
parse than `if not x ... else ...`.
SSHConnection.put() uses an scp call that will overwrite existing
directories as well as files.  shutil.copytree() isn't so aggressive.
create-sibling does rely on put() being able to handle existing and
destination directories in some cases (e.g., "reconfigure").  Fall
back to `cp` so we can handle this case.
Using "localhost" for the default sibling name for local paths is
fine, but let's use "local" instead to distinguish it from the similar
but not-quite-the-same case of a localhost:// SSH URL.
Rewrite to avoid the confusing imperative mood, and replace a comma
with a period while at it.
This debug message isn't as useful for the local case, because the
name is always the same, but there's no harm in logging it, and in
general it's good to keep the SSH/local code paths as close as
possible.
The upstream code does "if not sshurl: <set it>".  It doesn't make
much sense to sprinkle the codebase with assert's for every such case.
Also, get_connection() will fail quickly when passed None and with a
clearer error message.
In the next commit, we'll call resolve_path() within the "not
ssh_sibling" branch to resolve the path against the dataset.  We need
to resolve the path _before_ defaulting a null target_dir to
sibling_ri.path.
The current convention for relative path handling is that the path is
relative to the dataset if an instance is given to a command and
relative to the current working directory in all other cases.  Apply
this treatment to a local path given as `sshurl`.
When working with an SSH sibling, --target-dir is taken as relative to
the user's home directory.  In the context of a local path is given
for the SSH URL, this behavior doesn't make sense.  Instead, tack the
target directory onto the "URL".
This covers all the create-sibling tests except for test_invalid_call
and the not-so-simple test_target_ssh_simple.
@codecov
Copy link

codecov bot commented Feb 25, 2020

Codecov Report

Merging #4187 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4187      +/-   ##
==========================================
- Coverage   89.26%   89.23%   -0.03%     
==========================================
  Files         275      275              
  Lines       36036    36119      +83     
==========================================
+ Hits        32166    32232      +66     
- Misses       3870     3887      +17     
Impacted Files Coverage Δ
datalad/downloaders/http.py 70.91% <0.00%> (-2.79%) ⬇️
datalad/downloaders/tests/test_http.py 58.39% <0.00%> (-2.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2342cb4...154e222. Read the comment docs.

@mih
Copy link
Member

mih commented Feb 25, 2020

Looking good and working. Minor issue #4188 is long part of the establishment. Merging. Thx @kyleam !

@mih mih merged commit ef33313 into datalad:master Feb 25, 2020
14 of 17 checks passed
@kyleam kyleam deleted the resurrect-create-sibling-local branch Feb 25, 2020
@yarikoptic yarikoptic modified the milestones: 0.12.3, 0.12.x Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants