commands/update: add --rev option by ilgooz · Pull Request #2993 · treeverse/dvc

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

to update with a specific git revision.

please see updates on the docs treeverse/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

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

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 <jorgeorpinel@users.noreply.github.com> Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>

* 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!

Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>

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

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

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

LGTM, but test_update_rev needs to be modified

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

+        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

    )
    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

+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

+
+
+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

+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

+        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

+    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

Comment thread dvc/dependency/repo.py Outdated

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

Comment thread tests/unit/command/test_update.py Outdated

cosmetic updates

beab3d5

ilgooz mentioned this pull request Jan 2, 2020

command-reference/update: add docs for new --rev option treeverse/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

Comment thread dvc/repo/update.py Outdated

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

Comment thread dvc/command/update.py Outdated

jorgeorpinel reviewed Jan 2, 2020

View reviewed changes

Comment thread tests/unit/command/test_update.py Outdated

efiop reviewed Jan 2, 2020

View reviewed changes

Comment thread dvc/dependency/base.py Outdated

efiop suggested changes Jan 2, 2020

View reviewed changes

Comment thread dvc/stage.py Outdated

Comment thread tests/func/test_update.py Outdated

Comment thread tests/func/test_update.py Outdated

Comment thread tests/func/test_update.py Outdated

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

commands/update: cosmetic updates & update docs

386b25e

Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com> Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>

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

Comment thread tests/func/test_update.py Outdated

commands/update: import path of a test

f3eb2cd

Co-Authored-By: Ruslan Kuprieiev <kupruser@gmail.com>

Suor suggested changes Jan 6, 2020

View reviewed changes

Comment thread tests/unit/command/test_update.py Outdated

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 Suor, efiop 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

	"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):

Conversation

ilgooz commented Dec 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shcheklein commented Dec 21, 2019

Uh oh!

ilgooz commented Dec 22, 2019

Uh oh!

Suor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ilgooz commented Jan 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilgooz commented Jan 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgeorpinel left a comment

Choose a reason for hiding this comment

Uh oh!

ilgooz commented Jan 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiop commented Jan 4, 2020

Uh oh!

ilgooz commented Jan 5, 2020

Uh oh!

Uh oh!

ilgooz commented Jan 5, 2020

Uh oh!

Uh oh!

pared commented Jan 7, 2020

Uh oh!

efiop Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Jan 15, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Jan 15, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 15, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

ghost Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

efiop commented Jan 16, 2020

Uh oh!

efiop commented Jan 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

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