-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
move: validate new stage before move #5953
Conversation
@@ -330,7 +330,7 @@ def validate_state( | |||
def validate_kwargs(single_stage: bool = False, fname: str = None, **kwargs): | |||
"""Prepare, validate and process kwargs passed from cli""" | |||
cmd = kwargs.get("cmd") | |||
if not cmd: | |||
if not cmd and not single_stage: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this should be a separate data_source
kwarg instead of using single_stage
? @skshetry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still support stages in .dvc
file, so it's not just about data_source
. Maybe in 3.0? π
def test_move_output_overlap(tmp_dir, dvc): | ||
from dvc.exceptions import OverlappingOutputPathsError | ||
|
||
tmp_dir.dvc_gen({"foo": "foo", "dir": {"bar": "bar"}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this generate dir.dvc
or bar.dvc
? Should this test that dir.dvc
does not change (and maybe that dir/bar
still exists)?
Also, should both of these tests check that the .gitignore
files haven't changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generates dir.dvc
.
Regarding not making gitignore changes on failed stage creation, we check for that in other func tests, we shouldn't need to duplicate that here.
β 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.
Thank you for the contribution - we'll try to review it as soon as possible. π
Will close #5886
dvc move
now usesrepo.stage.create
to create the new destination stage instead of just modifying the existingstage.path
in order to do the necessary validation (check if dest overlaps with existing output, new.dvc
file would be gitignored, etc)