Skip to content

Conversation

@dtrifiro
Copy link
Contributor

fixes #7638

@dtrifiro dtrifiro requested a review from a team as a code owner April 28, 2022 10:28
@dtrifiro dtrifiro requested review from efiop and skshetry April 28, 2022 10:28
@dtrifiro dtrifiro force-pushed the fix/import-url-dir branch from cd24f77 to ff22c3a Compare April 28, 2022 10:30
@dtrifiro dtrifiro changed the title fix import-url for directories gs: fix import-url for directories Apr 28, 2022
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

We should be catching this external data stuff in the cloud tests (https://github.com/iterative/dvc-gs).
Seems that we are not really testing external data at all?
Similar thing happened in #7563

@efiop
Copy link
Contributor

efiop commented Apr 28, 2022

@daavoo Indeed, we specifically skip import_dir test in https://github.com/iterative/dvc-gs/blob/564592e28be38c8d45f7c04f0b07d0065a65f1b2/dvc_gs/tests/test_dvc.py#L39 I think this is for legacy reasons from way back when we used etags for dir hashes. We now use regular md5s, so might be able to make it work for gs.

@efiop
Copy link
Contributor

efiop commented Apr 28, 2022

@dtrifiro The testing for gs is a bit complicated, because we are in the middle of creating plugins, so the tests run in https://github.com/iterative/dvc-gs (they have the creds and stuff there). If you don't happen to have set it up before, you could create a test PR in dvc-gs and make it install dvc from your fork and run the tests there, just to confirm that it works before merging this.

@dtrifiro dtrifiro marked this pull request as draft April 28, 2022 11:22
@dtrifiro
Copy link
Contributor Author

dtrifiro commented Apr 28, 2022

Seems there are some failures. Will mark this PR as draft and investigate later

@dtrifiro dtrifiro changed the title gs: fix import-url for directories [WIP] gs: fix import-url for directories Apr 28, 2022
@dberenbaum
Copy link
Contributor

We should be catching this external data stuff in the cloud tests (https://github.com/iterative/dvc-gs).
Seems that we are not really testing external data at all?
Similar thing happened in #7563

Should we create a separate issue for this?

@daavoo
Copy link
Contributor

daavoo commented Jun 1, 2022

This needs to be moved to https://github.com/iterative/dvc-objects

@dtrifiro
Copy link
Contributor Author

dtrifiro commented Jun 8, 2022

Closing this as this was merged in treeverse/dvc-objects#41. Just need to wait for dvc-data to be bumped

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-url: importing directory from GCS bucket downloads to parent directory

4 participants