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

add, import-url: always resolve the output in the local path for --to-remote #5473

Merged
merged 3 commits into from
Feb 19, 2021

Conversation

isidentical
Copy link
Contributor

@isidentical isidentical commented Feb 16, 2021

During the discussions on #5445, it seemed --external is redundantly needed when trying to add an absolute path in the local system without -o. This wasn't the case for remote locations, such as s3:// or azure://. So this PR forbids the usage of --external with to-remote/to-cache and normalizes absolute system paths just like other remote providers.

@isidentical isidentical force-pushed the to-remote-output-resolver branch from 60ad639 to b457d98 Compare February 16, 2021 11:41
tests/func/test_add.py Outdated Show resolved Hide resolved
@@ -357,7 +357,7 @@ def resolve_output(inp, out):
return ret


def resolve_paths(repo, out):
def resolve_paths(repo, out, always_local=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, always_local is a high-level concept. Here, it does not convey what it does properly: using the base path as an output name. But, I don't have a good name to suggest either (that is succinct), so I'm fine with it as well. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also couldn't think of a better one 🤔

@isidentical isidentical requested a review from skshetry February 18, 2021 10:09
@efiop efiop merged commit 8102ede into iterative:master Feb 19, 2021
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