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

update: forbid updating repo-imports with --to-remote #5475

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

isidentical
Copy link
Contributor

@isidentical isidentical commented Feb 16, 2021

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

@efiop efiop merged commit e1a193f into iterative:master Feb 22, 2021
@jorgeorpinel
Copy link
Contributor

Hi @isidentical I see there's a docs PR for this (https://github.com/iterative/dvc.org/pull/2175/files), thanks!

I'm just not sure this was mentioned completely? What was forbidden exactly? It seems like update works perfectly well with --to-remote imports. Maybe I just don't understand this PR.

Comment on lines +10 to +12
raise InvalidArgumentError(
"Can't update a repo import with --to-remote"
)
Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 23, 2021

Choose a reason for hiding this comment

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

What's a repo import? Got it.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 23, 2021

Choose a reason for hiding this comment

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

BTW I'm not sure that term will mean anything for users. If that exception is a user message I would rephrase to something more explicit like "Data imported with dvc import can't be updated --to-remote." or "Only data imported with dvc import-url --to-remote can be updated --to-remote." β€” or whatever is most correct.

@jorgeorpinel
Copy link
Contributor

Oh I see "repo import" means a dvc import (not import-url). OK please see iterative/dvc.org#2175 (review)

@isidentical
Copy link
Contributor Author

Oh I see "repo import" means a dvc import (not import-url). OK please see iterative/dvc.org#2175 (review)

I agree that the error message is a bit term-focused, even though it is the correct term. Let's just use Only data imported with dvc import-url can be updated with --to-remote.

@jorgeorpinel
Copy link
Contributor

Yes or maybe more general like "Data imported from other DVC or Git repositories can't be updated to-remote." to avoid terminology completely. Although maybe mentioning the exact command is better. Up to you

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 25, 2021

p.s. is there a core issue to keep track of these ideas?

@isidentical
Copy link
Contributor Author

p.s. is there a core issue to keep track of these ideas?

nop, though feel free to just create an issue and CC me

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