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

commands/update: add --rev option #2993

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion dvc/command/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ def run(self):
ret = 0
for target in self.args.targets:
try:
self.repo.update(target)
self.repo.update(
target,
rev=self.args.rev,
)
except DvcException:
logger.exception("failed to update '{}'.".format(target))
ret = 1
Expand All @@ -35,4 +38,7 @@ def add_parser(subparsers, parent_parser):
update_parser.add_argument(
"targets", nargs="+", help="DVC-files to update."
)
update_parser.add_argument(
"--rev", nargs="?", help="DVC repository git revision."
ilgooz marked this conversation as resolved.
Show resolved Hide resolved
)
update_parser.set_defaults(func=CmdUpdate)
2 changes: 1 addition & 1 deletion dvc/dependency/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@ class DependencyBase(object):
IsNotFileOrDirError = DependencyIsNotFileOrDirError
IsStageFileError = DependencyIsStageFileError

def update(self):
def update(self, rev):
ilgooz marked this conversation as resolved.
Show resolved Hide resolved
pass
7 changes: 5 additions & 2 deletions dvc/dependency/repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ def download(self, to):
self.def_path, self.def_repo[self.PARAM_URL]
)

def update(self):
with self._make_repo(rev_lock=None) as repo:
def update(self, rev):
ilgooz marked this conversation as resolved.
Show resolved Hide resolved
if not rev:
rev = self.def_repo.get("rev")
Comment on lines +126 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to do that. See _make_rev, where it will merge your args with def_repo anyways.


with self._make_repo(rev=rev, rev_lock=None) as repo:
self.def_repo[self.PARAM_REV_LOCK] = repo.scm.get_rev()
7 changes: 7 additions & 0 deletions dvc/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -353,3 +353,10 @@ def __init__(self, path, repo):
" neighther as an output nor a git-handled file."
)
super(PathMissingError, self).__init__(msg.format(path, repo))


class UpdateWithRevNotPossibleError(DvcException):
def __init__(self):
super(UpdateWithRevNotPossibleError, self).__init__(
"Revision option (--rev) is not a feature of non-Git sources."
)
10 changes: 8 additions & 2 deletions dvc/repo/update.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
from dvc.exceptions import UpdateWithRevNotPossibleError

from . import locked


@locked
def update(self, target):
def update(self, target, rev=None):
from dvc.stage import Stage

stage = Stage.load(self, target)
stage.update()

if not stage.is_repo_import and rev:
raise UpdateWithRevNotPossibleError()

ilgooz marked this conversation as resolved.
Show resolved Hide resolved
stage.update(rev=rev)

stage.dump()
4 changes: 2 additions & 2 deletions dvc/stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -395,11 +395,11 @@ def reproduce(self, interactive=False, **kwargs):

return self

def update(self):
def update(self, rev=None):
if not self.is_repo_import and not self.is_import:
raise StageUpdateError(self.relpath)

self.deps[0].update()
self.deps[0].update(rev)
locked = self.locked
self.locked = False
try:
Expand Down
47 changes: 47 additions & 0 deletions tests/func/test_update.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import filecmp
import os
import shutil
import pytest

from dvc.exceptions import UpdateWithRevNotPossibleError
from dvc.external_repo import clean_repos
from dvc.repo import Repo

Expand Down Expand Up @@ -72,3 +74,48 @@ def test_update_import_url(repo_dir, dvc_repo):
assert os.path.exists(dst)
assert os.path.isfile(dst)
assert filecmp.cmp(src, dst, shallow=False)


def test_update_rev(dvc_repo, erepo):
src = "version"
dst = src

stage = dvc_repo.imp(erepo.root_dir, src, dst, rev="branch")

# update data
repo = Repo(erepo.root_dir)

saved_dir = os.getcwd()
os.chdir(erepo.root_dir)
ilgooz marked this conversation as resolved.
Show resolved Hide resolved

repo.scm.checkout("master")
os.unlink("version")
erepo.create("version", "updated")
repo.add("version")
repo.scm.add([".gitignore", "version.dvc"])
repo.scm.commit("updated")

repo.scm.close()
ilgooz marked this conversation as resolved.
Show resolved Hide resolved

os.chdir(saved_dir)
ilgooz marked this conversation as resolved.
Show resolved Hide resolved

# Caching in external repos doesn't see upstream updates within single
# cli call, so we need to clean the caches to see the changes.
clean_repos()

dvc_repo.update(stage.path, "master")

with open(dst, "r+") as fobj:
assert fobj.read() == "updated"


def test_update_rev_non_git_failure(repo_dir, dvc_repo):
Copy link

Choose a reason for hiding this comment

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

We are not using repo_dir and dvc_repo anymore, instead, we use tmp_dir or dvc.
This is not crucial, but it would be nice if you could read the docstring from tests/dir_helpers.py.
Sorry for the extra work, @ilgooz 😞

Copy link

Choose a reason for hiding this comment

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

Here's a suggestion, to save you some time:

def test_update_rev_non_git_failure(tmp_dir, dvc):
    tmp_dir.gen("file", "text")
    stage = dvc.imp_url("file", "imported")

    with pytest.raises(UpdateWithRevNotPossibleError):
        dvc_repo.update(stage.path, rev="dev")

Looks neat)

src = "file"
dst = src + "_imported"

shutil.copyfile(repo_dir.FOO, src)

stage = dvc_repo.imp_url(src, dst)

with pytest.raises(UpdateWithRevNotPossibleError):
dvc_repo.update(stage.path, rev="dev")
24 changes: 22 additions & 2 deletions tests/unit/command/test_update.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,35 @@
import logging

from dvc.cli import parse_args
from dvc.command.update import CmdUpdate
from dvc.exceptions import UpdateWithRevNotPossibleError


def test_update(dvc_repo, mocker):
targets = ["target1", "target2", "target3"]
cli_args = parse_args(["update"] + targets)
cli_args = parse_args(["update", "--rev", "develop"] + targets)
assert cli_args.func == CmdUpdate
cmd = cli_args.func(cli_args)
m = mocker.patch("dvc.repo.Repo.update")

assert cmd.run() == 0

calls = [mocker.call(target) for target in targets]
calls = [mocker.call(target, rev="develop") for target in targets]
m.assert_has_calls(calls)


def test_update_rev_failed(mocker, caplog, dvc_repo):
ilgooz marked this conversation as resolved.
Show resolved Hide resolved
targets = ["target1", "target2", "target3"]
cli_args = parse_args(["update", "--rev", "develop"] + targets)
assert cli_args.func == CmdUpdate

cmd = cli_args.func(cli_args)
with mocker.patch.object(
cmd.repo, "update", side_effect=UpdateWithRevNotPossibleError()
):
with caplog.at_level(logging.ERROR, logger="dvc"):
assert cmd.run() == 1
expected_error = (
"Revision option (--rev) is not a feature of non-Git sources."
jorgeorpinel marked this conversation as resolved.
Show resolved Hide resolved
)
assert expected_error in caplog.text