commands/update: add --rev option #2993

ilgooz · 2019-12-21T21:50:06Z

to update with a specific git revision.

please see updates on the docs iterative/dvc.org#890.

resolves #2849.

--

❗ 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. 🙏

to update with a specific git revision

shcheklein · 2019-12-21T23:13:05Z

@ilgooz looks like you need to install the pre-commit hook that takes care of the code style - please, take a look at the contributing guide.

ilgooz · 2019-12-22T09:42:41Z

@ilgooz looks like you need to install the pre-commit hook that takes care of the code style - please, take a look at the contributing guide.

Thanks Ivan, I'll do that and also some fixes, and add some tests!

Suor

PR is incomplete:

format things, e.g. with pre-commit hooks, to run tests
need a functional test for dvc update --rev
need to check that dvc update --rev ... shows an appropriate error for a import-url stage

dvc/dependency/repo.py

tests/unit/command/test_update.py

ilgooz · 2020-01-02T12:44:27Z

@ilgooz looks like you need to install the pre-commit hook that takes care of the code style - please, take a look at the contributing guide.

PR is incomplete:

format things, e.g. with pre-commit hooks, to run tests

need a functional test for dvc update --rev

need to check that dvc update --rev ... shows an appropriate error for a import-url stage

done by 95db0ea

dvc/repo/update.py

dvc/command/update.py

tests/unit/command/test_update.py

dvc/dependency/base.py

dvc/stage.py

tests/func/test_update.py

ilgooz · 2020-01-04T18:33:11Z

Hi guys, big thanks for your encouraging collaboration, I'll do the requested changes. Can you please participate to the discussion at the following link as well? #2849 (comment)

Co-Authored-By: Jorge Orpinel <[email protected]> Co-Authored-By: Ruslan Kuprieiev <[email protected]>

* small refactoring on the related test to use new testing util.

jorgeorpinel

LGTM

ilgooz · 2020-01-04T23:50:22Z

Any ideas of how to get Travis check back? Should I split the string into two lines?

https://travis-ci.com/iterative/dvc/jobs/272434824

tests/unit/command/test_update.py:32:80: E501 line too long (93 > 79 characters)
The command "./scripts/ci/check_patch.sh" exited with 1.

efiop · 2020-01-04T23:56:06Z

@ilgooz Yes, please split it somehow.

ilgooz · 2020-01-05T00:34:29Z

cc @Suor up for reviews!

tests/func/test_update.py

Co-Authored-By: Ruslan Kuprieiev <[email protected]>

ilgooz · 2020-01-05T01:05:44Z

Tests won't pass again, I'll check this tomorrow!

tests/unit/command/test_update.py

pared · 2020-01-07T10:41:50Z

LGTM, but test_update_rev needs to be modified

efiop · 2020-01-15T18:46:10Z

dvc/dependency/repo.py

+        if not rev:
+            rev = self.def_repo.get("rev")


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

ghost · 2020-01-15T19:31:23Z

dvc/command/imp.py

@@ -53,6 +53,6 @@ def add_parser(subparsers, parent_parser):
        "-o", "--out", nargs="?", help="Destination path to download files to"
    )
    import_parser.add_argument(
-        "--rev", nargs="?", help="Git revision (e.g. branch, tag, SHA)"
+        "--rev", nargs="?", help="Git revision in repository to update from."


@ilgooz , why removing the examples? Not everyone is familiar with the term revision)

ghost · 2020-01-15T19:40:22Z

dvc/exceptions.py

+class UpdateWithRevNotPossibleError(DvcException):
+    def __init__(self):
+        super(UpdateWithRevNotPossibleError, self).__init__(
+            "Revision option (`--rev`) is not a feature of non-Git sources."


Usually it is easier to read without double negations:
not a feature of non-Git sources -> only a feature of Git sources

Suggested change

"Revision option (`--rev`) is not a feature of non-Git sources."

"Revision option (`--rev`) is supported only for Git sources."

ghost · 2020-01-15T19:43:09Z

tests/func/test_update.py

+
+
+def test_update_rev(tmp_dir, dvc, erepo_dir, monkeypatch):
+    with monkeypatch.context() as m:


@ilgooz , if the reason for using monkeypatch is to chdir to erepo_dir, you can use instead:

Suggested change

with monkeypatch.context() as m:

with erepo_dir.chdir():

ghost · 2020-01-15T19:45:17Z

tests/func/test_update.py

+def test_update_rev(tmp_dir, dvc, erepo_dir, monkeypatch):
+    with monkeypatch.context() as m:
+        m.chdir(fspath(erepo_dir))
+        erepo_dir.scm.checkout("new_branch", create_new=True)


Another useful helper to change to a branch temporarily:

Suggested change

erepo_dir.scm.checkout("new_branch", create_new=True)

with erepo_dir.branch("new_branch", new=True):

# ...

ghost · 2020-01-15T19:46:18Z

tests/func/test_update.py

+        erepo_dir.scm.checkout("new_branch", create_new=True)
+        erepo_dir.dvc_gen("foo", "foo content", commit="create foo on branch")
+        erepo_dir.scm.checkout("master")
+        erepo_dir.scm.checkout("new_branch_2", create_new=True)


Same here:

Suggested change

erepo_dir.scm.checkout("new_branch_2", create_new=True)

with erepo_dir.branch("new_branch_2", new=True):

ghost · 2020-01-15T19:49:51Z

tests/func/test_update.py

+    assert (tmp_dir / "foo_imported").read_text() == "foo content 2"
+
+
+def test_update_rev_non_git_failure(repo_dir, dvc_repo):


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 😞

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)

efiop · 2020-01-16T16:14:07Z

Test changes are caused by this PR existing through the period of us refactoring our test suite 🙂Sorry for the inconvenience.

efiop · 2020-01-20T12:51:23Z

Closing as stale.

commands/update: add --rev option

b285780

to update with a specific git revision

shcheklein requested review from Suor and efiop December 21, 2019 23:11

weekly-digest bot mentioned this pull request Dec 22, 2019

Weekly Digest (15 December, 2019 - 22 December, 2019) #2996

Closed

Suor suggested changes Dec 25, 2019

View reviewed changes

dvc/dependency/repo.py Outdated Show resolved Hide resolved

weekly-digest bot mentioned this pull request Dec 29, 2019

Weekly Digest (22 December, 2019 - 29 December, 2019) #3013

Closed

commands/update: complete --rev feature and add tests

95db0ea

ilgooz commented Jan 2, 2020

View reviewed changes

tests/unit/command/test_update.py Outdated Show resolved Hide resolved

cosmetic updates

beab3d5

ilgooz mentioned this pull request Jan 2, 2020

command-reference/update: add docs for new --rev option iterative/dvc.org#890

Closed

ilgooz marked this pull request as ready for review January 2, 2020 13:20

ilgooz requested a review from Suor January 2, 2020 13:27

efiop reviewed Jan 2, 2020

View reviewed changes

dvc/repo/update.py Outdated Show resolved Hide resolved

commands/update: do validation for update with --rev inside Stage

9d68c8a

ilgooz requested a review from efiop January 2, 2020 17:10

jorgeorpinel reviewed Jan 2, 2020

View reviewed changes

dvc/command/update.py Outdated Show resolved Hide resolved

jorgeorpinel reviewed Jan 2, 2020

View reviewed changes

tests/unit/command/test_update.py Outdated Show resolved Hide resolved

efiop reviewed Jan 2, 2020

View reviewed changes

dvc/dependency/base.py Outdated Show resolved Hide resolved

efiop suggested changes Jan 2, 2020

View reviewed changes

dvc/stage.py Outdated Show resolved Hide resolved

tests/func/test_update.py Outdated Show resolved Hide resolved

tests/func/test_update.py Outdated Show resolved Hide resolved

tests/func/test_update.py Outdated Show resolved Hide resolved

ilgooz and others added 3 commits January 4, 2020 22:45

commands/update: cosmetic updates & update docs

386b25e

Co-Authored-By: Jorge Orpinel <[email protected]> Co-Authored-By: Ruslan Kuprieiev <[email protected]>

commands/update: cosmetic updates related to --rev option

32494bb

* small refactoring on the related test to use new testing util.

commands/update: update docs related to --rev option

a9fa797

restyled-io bot mentioned this pull request Jan 4, 2020

Restyle commands/update: add --rev option #3057

Closed

jorgeorpinel approved these changes Jan 4, 2020

View reviewed changes

ilgooz added 2 commits January 5, 2020 00:15

commands/update: satisfy linter for code related to --rev option

9a04fbd

commands/update: cosmetic updates related to --rev option

f9e5cc9

ilgooz requested a review from efiop January 4, 2020 23:46

ilgooz added 2 commits January 5, 2020 03:03

commands/update: satisfy linter

4fe900f

commands/update: fix tests

5f61f0c

efiop reviewed Jan 5, 2020

View reviewed changes

tests/func/test_update.py Outdated Show resolved Hide resolved

commands/update: import path of a test

f3eb2cd

Co-Authored-By: Ruslan Kuprieiev <[email protected]>

Suor suggested changes Jan 6, 2020

View reviewed changes

tests/unit/command/test_update.py Outdated Show resolved Hide resolved

weekly-digest bot mentioned this pull request Jan 12, 2020

Weekly Digest (5 January, 2020 - 12 January, 2020) #3119

Closed

ilgooz added 3 commits January 15, 2020 12:16

commands: --rev option: fix tests & rm an unneeded one

2abf41c

Merge branch 'master' into feature/update-rev

d2072b9

fix merge master

f56efc6

restyled-io bot mentioned this pull request Jan 15, 2020

Restyle commands/update: add --rev option #3160

Closed

satisfy linter

e6a56f3

ilgooz requested review from efiop, Suor and jorgeorpinel January 15, 2020 17:24

efiop reviewed Jan 15, 2020

View reviewed changes

ghost reviewed Jan 15, 2020

View reviewed changes

efiop closed this Jan 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commands/update: add --rev option #2993

commands/update: add --rev option #2993

ilgooz commented Dec 21, 2019 •

edited

Loading

shcheklein commented Dec 21, 2019

ilgooz commented Dec 22, 2019

Suor left a comment •

edited

Loading

ilgooz commented Jan 2, 2020

ilgooz commented Jan 4, 2020 •

edited

Loading

jorgeorpinel left a comment

ilgooz commented Jan 4, 2020 •

edited

Loading

efiop commented Jan 4, 2020

ilgooz commented Jan 5, 2020

ilgooz commented Jan 5, 2020

pared commented Jan 7, 2020

efiop Jan 15, 2020

ghost Jan 15, 2020 •

edited by ghost

Loading

ghost Jan 15, 2020

ghost Jan 15, 2020

ghost Jan 15, 2020 •

edited by ghost

Loading

ghost Jan 15, 2020 •

edited by ghost

Loading

ghost Jan 15, 2020

ghost Jan 15, 2020

efiop commented Jan 16, 2020

efiop commented Jan 20, 2020

	"Revision option (`--rev`) is not a feature of non-Git sources."
	"Revision option (`--rev`) is supported only for Git sources."



		def test_update_rev(tmp_dir, dvc, erepo_dir, monkeypatch):
		with monkeypatch.context() as m:

	erepo_dir.scm.checkout("new_branch", create_new=True)
	with erepo_dir.branch("new_branch", new=True):
	# ...

	erepo_dir.scm.checkout("new_branch_2", create_new=True)
	with erepo_dir.branch("new_branch_2", new=True):

		assert (tmp_dir / "foo_imported").read_text() == "foo content 2"


		def test_update_rev_non_git_failure(repo_dir, dvc_repo):

commands/update: add --rev option #2993

commands/update: add --rev option #2993

Conversation

ilgooz commented Dec 21, 2019 • edited Loading

shcheklein commented Dec 21, 2019

ilgooz commented Dec 22, 2019

Suor left a comment • edited Loading

Choose a reason for hiding this comment

ilgooz commented Jan 2, 2020

ilgooz commented Jan 4, 2020 • edited Loading

jorgeorpinel left a comment

Choose a reason for hiding this comment

ilgooz commented Jan 4, 2020 • edited Loading

efiop commented Jan 4, 2020

ilgooz commented Jan 5, 2020

ilgooz commented Jan 5, 2020

pared commented Jan 7, 2020

efiop Jan 15, 2020

Choose a reason for hiding this comment

ghost Jan 15, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jan 15, 2020

Choose a reason for hiding this comment

ghost Jan 15, 2020

Choose a reason for hiding this comment

ghost Jan 15, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jan 15, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Jan 15, 2020

Choose a reason for hiding this comment

ghost Jan 15, 2020

Choose a reason for hiding this comment

efiop commented Jan 16, 2020

efiop commented Jan 20, 2020

ilgooz commented Dec 21, 2019 •

edited

Loading

Suor left a comment •

edited

Loading

ilgooz commented Jan 4, 2020 •

edited

Loading

ilgooz commented Jan 4, 2020 •

edited

Loading

ghost Jan 15, 2020 •

edited by ghost

Loading

ghost Jan 15, 2020 •

edited by ghost

Loading

ghost Jan 15, 2020 •

edited by ghost

Loading