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

import: allow importing from non-DVC git repositories #3020

Merged
merged 3 commits into from
Jan 5, 2020
Merged

import: allow importing from non-DVC git repositories #3020

merged 3 commits into from
Jan 5, 2020

Conversation

chatcannon
Copy link
Contributor

@chatcannon chatcannon commented Dec 30, 2019

Fixes #2977

When accessing an external_repo, create a '.dvc' directory if
there is not already one there.

  • ❗ Have you followed the guidelines in the Contributing to DVC list?

  • πŸ“– Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

  • ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

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

dvc/external_repo.py Outdated Show resolved Hide resolved
@efiop
Copy link
Contributor

efiop commented Dec 30, 2019

Also, no tests? Your PR even broke existing tests. Is this change ready for review? Or is it a WIP?

@chatcannon
Copy link
Contributor Author

No, it's not ready for review yet, sorry

@chatcannon chatcannon changed the title import: allow importing from non-DVC git repositories WIP: import: allow importing from non-DVC git repositories Dec 31, 2019
Since this function does not create a `Repo` class, it can be called
on a git repo with no '.dvc' directory
Fixes #2977

Refactor `_copy_if_git_file` to use `cached_clone` directly, so
that copying still works even if the source repo does not have a
'.dvc' subdirectory.
@chatcannon
Copy link
Contributor Author

The DeepSource check wants me to group imports, but this goes against the style of the existing code. What should I do?

https://deepsource.io/gh/iterative/dvc/run/9af8c44e-672d-48c3-84e0-032d14abb837/#python

@chatcannon chatcannon changed the title WIP: import: allow importing from non-DVC git repositories import: allow importing from non-DVC git repositories Dec 31, 2019
@chatcannon
Copy link
Contributor Author

Regarding documentation, I could edit https://github.com/iterative/dvc.org/blob/master/static/docs/command-reference/import.md and change "DVC repository" to "DVC or Git repository" but I'm not sure if that's really necessary.

@ghost
Copy link

ghost commented Dec 31, 2019

The DeepSource check wants me to group imports, but this goes against the style of the existing code. What should I do?

@chatcannon , you don't need to make DeepSource pass, use it more as a guideline (that you might not follow).

@ghost
Copy link

ghost commented Dec 31, 2019

Regarding documentation, I could edit https://github.com/iterative/dvc.org/blob/master/static/docs/command-reference/import.md and change "DVC repository" to "DVC or Git repository" but I'm not sure if that's really necessary.

@jorgeorpinel , what do you suggest?

@efiop
Copy link
Contributor

efiop commented Jan 1, 2020

Regarding docs, sure we need to note that this now works on non-dvc repos, there is no doubt about it and it is clearly necessary. πŸ™‚

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jan 3, 2020

Yes I think docs definitely need to be updated so that the dvc import command reference reflects the changes in this PR. You can open a separate iterative/PR on dvc.org for that and we'll review it there and help finish it if necessary. Thanks for the heads up, guys.

change "DVC repository" to "DVC or Git repository"

@chatcannon sounds like a good first step but I imagine there needs to be more changes, would have to read the cmd ref thoroughly and think about it. Please open the PR and we'll get that review process stated.

@efiop efiop requested review from Suor, pared and a user January 4, 2020 02:32
Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Sorry for a late review. This is neat!

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

LGTM

@efiop efiop merged commit f935748 into iterative:master Jan 5, 2020
@efiop
Copy link
Contributor

efiop commented Jan 5, 2020

@chatcannon Please either create an issue for adjusting docs for this or, even better, send a PR to adjust the docs. Thanks! πŸ™‚

@ghost
Copy link

ghost commented Jan 5, 2020

#3020 (review)
oops, wrong click πŸ™ˆ (didn't mean to review it twice)

@jorgeorpinel
Copy link
Contributor

I went ahead and created iterative/dvc.org/issues/898 because I feel it may be forgotten now that this PR is merged without the docs checkbox. Feel free to take that one, @chatcannon. Thanks πŸ™‚

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.

import: Handle non-DVC Git repositories
4 participants