remote: verify repository exists when level requires it #3194
Conversation
tests/func/test_remote.py
Outdated
| ], | ||
| ) | ||
| def test_commands_outside_dvc_repository(tmp_dir, cmd): | ||
| assert 253 == main(cmd) |
There was a problem hiding this comment.
253 is returned when NotDvcRepoError is raised.
There was a problem hiding this comment.
This issue is relevant for dvc config in general and should be fixed there instead. dvc_dir defaults to None and so https://github.com/iterative/dvc/blob/master/dvc/config.py#L261 loads empty configs with filename == None, adjusts them and then doesn't save them because of filename == None. Need a general approach here.
ghost
requested a review
from
| COMPILED_SCHEMA = Schema(SCHEMA) | ||
|
|
||
| def __init__(self, dvc_dir=None, validate=True): | ||
| def __init__(self, dvc_dir=None, validate=True, verify_repo=False): |
There was a problem hiding this comment.
Added a new parameter because I didn't want to mess up with any previous logic dealing with having an empty dvc_dir.
There was a problem hiding this comment.
@MrOutis This looks like a hack rather than a proper solution 🙂
There was a problem hiding this comment.
@efiop, could you explain? Not sure how to proceed with such observation 😅
There was a problem hiding this comment.
You are hacking your way through the bug instead of solving the bug. This verify_repo shouldn't be an option at all. Config should always raise if you try to modify in-repo config when there isn't one.
tests/func/test_remote.py
Outdated
| remote_config.modify("myremote", "gdrive_client_id", "xxx") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Solution is for config in general, but tests are for remote specifically, not nice 🙂Also not clear why are you testing CLI again and not API?
There was a problem hiding this comment.
My bad, that's a left over. Those tests are not relevant since they were replaced for the ones in test_config.py
There was a problem hiding this comment.
🙈 I got confused with the other pull-request:
https://github.com/iterative/dvc/pull/3193/files#diff-dcfb923229ccfbba150bdd77494f46eeR96-R97
There was a problem hiding this comment.
I prefer testing the CLI in this case, it is more complete, but if you want, I use the API.
There was a problem hiding this comment.
@MrOutis CLI is a dumb wrapper for API. We always test API, so let's do the same here. If you want to check that CLI returns proper codes on corresponding exceptions, just add a unit test to tests/unit/command/.
|
Ok, looks like there is no good quick and neat way to solve it, it will look like a hack in any case. What we need here is a redesign. 🙁 |
|
Closing for now, this will require a config redesign first. |
1 participant
Close #2309