[#4136] fixed recalculating relative paths in config

[godfryd]

Other paths beside gdrive_user_credentials_file and cache.dir should be recalculated.
Added the following ones:

• gdrive_service_account_p12_file_path
• credentialpath
• keyfile
• cert_path
• key_path

Will fix #4136.

[efiop]

Thanks for the PR! Looks great!

Could we slim down the tests to just test that the paths are adjusted properly when loading a config with these fields? Current tests do the job, but they are quite heavy for such a simple function. Also, I see a lot of code duplication in the tests, would it be possible to reasonably us e pytest parametrize helper?

[efiop]

Linter CI job is complaining about this one

Suggested change

	assert main(["remote", "add", "-d", remote_name, remote_url,]) == 0
	assert main(["remote", "add", "-d", remote_name, remote_url]) == 0

[godfryd]

fixed

[efiop]

Looks much better now! Thank you! 🙏

What do you think about creating instead a unit test(maybe -ish) for the Config class, to check that when it contains all of these fields - they are are all getting resolved properly when loading?

[godfryd]

I'm not sure. Current tests involve setting paths and loading them in a config. Proposed test would do only the second part.

[efiop]

Seems like this is testing how it is written to the config file, but not really how it is loaded. Or am I missing something?

[efiop]

@godfryd The setting part is pretty safe. You are currently using CLI and realoding the whole repo instance in order to test it, but you could really just do something like:

with config.edit() as conf:
    # set the options you want to test

# check if written paths are correct

# load config and check that it contains what we expect (relative paths are evaluated correctly)

which should be easily parametrizable (or could even shove everything into one test without parametrizations), simple and lightweight. What do you think?

[efiop]

Looks great! Please take a look at the windows tests, looks like there is a bug

2020-11-07T09:49:32.2041318Z     def test_load_relative_paths(dvc, field, remote_url):
2020-11-07T09:49:32.2041690Z         # set field to test
2020-11-07T09:49:32.2042032Z         with dvc.config.edit() as conf:
2020-11-07T09:49:32.2042463Z             conf["remote"]["test"] = {"url": remote_url, field: "file.txt"}
2020-11-07T09:49:32.2042793Z     
2020-11-07T09:49:32.2043269Z         # check if written paths are correct
2020-11-07T09:49:32.2043710Z         dvc_dir = dvc.config.dvc_dir
2020-11-07T09:49:32.2044307Z >       assert dvc.config["remote"]["test"][field] == os.path.join(
2020-11-07T09:49:32.2044735Z             dvc_dir, "..", "file.txt"
2020-11-07T09:49:32.2045003Z         )
2020-11-07T09:49:32.2045425Z E       AssertionError: assert 'C:\\Users\\r...\\../file.txt' == 'C:\\Users\\r...\..\\file.txt'
2020-11-07T09:49:32.2046433Z E         - C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\popen-gw3\test_load_relative_paths_cert_0\.dvc\..\file.txt
2020-11-07T09:49:32.2047086Z E         ?                                                                                                                         ^
2020-11-07T09:49:32.2047786Z E         + C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\popen-gw3\test_load_relative_paths_cert_0\.dvc\../file.txt
2020-11-07T09:49:32.2048624Z E         ?     

[efiop]

Looks great! Thank you! 🙏