Skip to content

RepoDependency: never changed#4209

Merged
efiop merged 1 commit into
treeverse:masterfrom
pared:4205_commit_imported
Jul 16, 2020
Merged

RepoDependency: never changed#4209
efiop merged 1 commit into
treeverse:masterfrom
pared:4205_commit_imported

Conversation

@pared
Copy link
Copy Markdown
Contributor

@pared pared commented Jul 15, 2020 •
edited
Loading

  • ❗ I have followed the Contributing to DVC checklist.

  • 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏
Fixes #4205

@pared pared force-pushed the 4205_commit_imported branch from d7cabf1 to 5d57706 Compare July 15, 2020 11:52
@pared pared changed the title [WIP] RepoDependency: never changed [WIP] RepoDependency: not changed on no checksum Jul 15, 2020
@pared pared force-pushed the 4205_commit_imported branch from 5d57706 to f2010f7 Compare July 15, 2020 13:29
@pared pared changed the title [WIP] RepoDependency: not changed on no checksum [WIP] RepoDependency: never changed Jul 15, 2020
@pared pared changed the title [WIP] RepoDependency: never changed RepoDependency: never changed Jul 15, 2020
@pared pared marked this pull request as ready for review July 15, 2020 13:30
@pared pared requested review from efiop, pmrowla and skshetry July 15, 2020 13:30
skshetry
skshetry approved these changes Jul 16, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

Do we need a test for this? Otherwise, looks good to me.

@pared
Copy link
Copy Markdown
Contributor Author

pared commented Jul 16, 2020

@skshetry
I have test with committing imported for that, though I am not sure whether we should include that:
This test checks a particular use case, which origin is more general - lack of proper checksum_changed() method.
On the other hand, I would assume that committing repository with imported repo is quite popular use case, so it makes sense to test it. I will re-add the test.

@pared pared force-pushed the 4205_commit_imported branch from f2010f7 to 5ea6ba3 Compare July 16, 2020 09:41
@pared pared force-pushed the 4205_commit_imported branch from 5ea6ba3 to 624f2d1 Compare July 16, 2020 09:42
@pared
Copy link
Copy Markdown
Contributor Author

pared commented Jul 16, 2020

@skshetry I created test

efiop
efiop reviewed Jul 16, 2020
View reviewed changes
Comment thread tests/func/test_commit.py
Comment on lines +108 to +109
with dvc.state:
assert stage.changed_entries() == ([], [], None)
Copy link
Copy Markdown
Contributor

@efiop efiop Jul 16, 2020

Choose a reason for hiding this comment

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

Won't it show up on dvc.status?

Copy link
Copy Markdown
Collaborator

@skshetry skshetry Jul 16, 2020

Choose a reason for hiding this comment

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

IIRC these are not used by status but by commit to show prompt message on what's changed.

Copy link
Copy Markdown
Contributor Author

@pared pared Jul 17, 2020

Choose a reason for hiding this comment

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

@skshetry is right, in case of commit we check whether changed_checksum for RepoDependency changed. Upon status, we actually get the checksum for the file from the external repo, to get information on whether it has been updated.

@efiop efiop merged commit e4dafb8 into treeverse:master Jul 16, 2020
@weekly-digest weekly-digest Bot mentioned this pull request Jul 19, 2020
Closed
@pared pared deleted the 4205_commit_imported branch January 4, 2022 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skshetry skshetry skshetry approved these changes

+2 more reviewers

@efiop efiop efiop left review comments

@pmrowla pmrowla pmrowla approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

dvc commit fails with files imported via dvc import

4 participants

@pared @pmrowla @efiop @skshetry