cli: config: add support for --show-origin by mrstegeman · Pull Request #5126 · treeverse/dvc

mrstegeman · 2020-12-17T20:03:55Z

Support the --show-origin option for config, which, similar to git,
prefixes each config option with the source file it originated from.

Fixes #5119

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

Support the --show-origin option for config, which, similar to git, prefixes each config option with the source file it originated from. Fixes treeverse#5119

shcheklein · 2020-12-17T21:45:47Z

@mrstegeman hey! 👋 Thanks for the PR! I see that you set the docs checkbox, but I don't see any issues/PRs in the docs repo? Is it still WIP?

Support the --show-origin option for config, which, similar to git, prefixes each config option with the source file it originated from. References treeverse/dvc#5119 References treeverse/dvc#5126

mrstegeman · 2020-12-17T22:22:57Z

Ah, apologies. I was thinking it was just documentation in this repo.

treeverse/dvc.org#2028

shcheklein · 2020-12-18T03:39:58Z

@mrstegeman no worries at all (the layout is not very usual indeed when you have a separate repo with docs). Thanks for the PR, we'll take a look asap!! Give us please a bit of time for this.

efiop

Hi @mrstegeman !

Thanks for the PR! 🙏 Looks good!

I totally forgot that we still didn't introduce --repo flag for dvc config and didn't switch to loading the merged config in dvc config without --repo/--local/--global/--system 🤦‍♂️ 🤦‍♂️ 🤦‍♂️ It is on our 2.0 todo list though. Which would make the task a bit harder :)

mrstegeman · 2020-12-18T18:02:13Z

I totally forgot that we still didn't introduce --repo flag for dvc config and didn't switch to loading the merged config in dvc config without --repo/--local/--global/--system 🤦‍♂️ 🤦‍♂️ 🤦‍♂️ It is on our 2.0 todo list though. Which would make the task a bit harder :)

Yep, that was the biggest difference I noticed versus git config --list. It shouldn't make the logic much more difficult, though.

efiop · 2020-12-18T18:05:14Z

Yep, that was the biggest difference I noticed versus git config --list. It shouldn't make the logic much more difficult, though.

@mrstegeman Would you be willing to introduce the changes for that? Our master branch is currently preparing for 2.0, so we can do backward-incompatible changes without any problems. But we could merge this first and then think about the next step too, no problem.

mrstegeman · 2020-12-22T07:41:45Z

@efiop The two changes are really independent of each other, but should be done separately. It probably makes the most sense to just merge this as is, and then figure out the logic to merge and print all of the configs in a separate PR. I'd be happy to take on the other PR as well, after this is merged.

mrstegeman · 2020-12-22T07:48:00Z

For my own reference: 2.0 checklist

efiop

@mrstegeman Sounds good!

Ok, this PR looks good, but we need to add a test that tests the output. Please see tests/func/test_config.py::test_config_remote for an example of using caplog.

karajan1001 · 2020-12-29T06:51:21Z

        self.assertEqual(ret, 0)
        self.assertTrue(self._contains(section, field, value, local))

+        ret = main(base + [section_field, value, "--show-origin"])


Can we parametrize these tests?

The tests are being reworked in #5179.

karajan1001 · 2020-12-29T07:42:39Z

I totally forgot that we still didn't introduce --repo flag for dvc config and didn't switch to loading the merged config in dvc config without --repo/--local/--global/--system 🤦‍♂️ 🤦‍♂️ 🤦‍♂️ It is on our 2.0 todo list though. Which would make the task a bit harder :)

@efiop
Excuse me, any details for the coming changes in 2.0? Currently dvc config --list and dvc config behave differently in default ( without --repo/--local/--global/--system)? dvc config only changes --repo config while dvc config --list shows a merged one?

efiop · 2020-12-29T22:06:57Z

I totally forgot that we still didn't introduce --repo flag for dvc config and didn't switch to loading the merged config in dvc config without --repo/--local/--global/--system man_facepalming man_facepalming man_facepalming It is on our 2.0 todo list though. Which would make the task a bit harder :)

@efiop
Excuse me, any details for the coming changes in 2.0? Currently dvc config --list and dvc config behave differently in default ( without --repo/--local/--global/--system)? dvc config only changes --repo config while dvc config --list shows a merged one?

No, dvc config --list and dvc config(in read mode) behave the same by default, but the problem is that they show repo level config by default, and not the merged config. But if you try to write something with it - it writes to repo config by default, which is correct. So in read mode we should use merged config and in write mode - continue using repo config by default. This is the same behavior as git config has.

mrstegeman · 2020-12-30T07:52:58Z

Apologies for the delay. I added a couple tests for the output.

efiop

Thank you!

efiop · 2020-12-30T22:55:58Z

@mrstegeman I've added --repo flag and changed dvc config behaviour in #5179 (we need it for the upcoming 2.0 soon anyway), but temporarily forbid the use of --show-origin without --system/--global/--repo/--local flag so you could continue your work on a proper implementation as we've discussed here before. Please feel free to send a PR for that 🙂

Builds on treeverse#5126 and treeverse#5184 by showing the full merged config when listing or getting config options. References treeverse#5126 References treeverse#5184 References treeverse/dvc.org#2028 Fixes treeverse#5119

Support the --show-origin option for config, which, similar to git, prefixes each config option with the source file it originated from. References treeverse/dvc#5119 References treeverse/dvc#5126

mrstegeman · 2020-12-31T09:38:36Z

@efiop Done, see #5188

* cli: config: show merged config with --show-origin Builds on #5126 and #5184 by showing the full merged config when listing or getting config options. References #5126 References #5184 References treeverse/dvc.org#2028 Fixes #5119 * Add additional tests. * Add test for merged config. * config: rename methods Co-authored-by: OLOLO ALALA <ruslan@iterative.ai>

* cli: config: add docs for --show-origin Support the --show-origin option for config, which, similar to git, prefixes each config option with the source file it originated from. References treeverse/dvc#5119 References treeverse/dvc#5126 * Update wording. * Add note about the options near config file locations. * Update content/docs/command-reference/config.md Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

mrstegeman added 2 commits December 17, 2020 10:59

cli: config: add support for --show-origin

14e16d0

Support the --show-origin option for config, which, similar to git, prefixes each config option with the source file it originated from. Fixes treeverse#5119

Reformat with black.

40b1d8c

mrstegeman mentioned this pull request Dec 17, 2020

cli: config: add docs for --show-origin treeverse/dvc.org#2028

Merged

shcheklein requested a review from efiop December 18, 2020 03:40

efiop reviewed Dec 18, 2020

View reviewed changes

Comment thread dvc/command/config.py Outdated

efiop reviewed Dec 18, 2020

View reviewed changes

Simplify logic for local config files.

d7238e9

efiop reviewed Dec 22, 2020

View reviewed changes

Comment thread dvc/command/config.py

karajan1001 reviewed Dec 29, 2020

View reviewed changes

efiop mentioned this pull request Dec 29, 2020

config: read merged config by default #5179

Merged

2 tasks

Add test for --show-config output.

87155e7

mrstegeman force-pushed the show-origin branch from 929e71d to 87155e7 Compare December 30, 2020 08:27

efiop reviewed Dec 30, 2020

View reviewed changes

Comment thread tests/func/test_config.py Outdated

Update tests/func/test_config.py

e6def37

efiop approved these changes Dec 30, 2020

View reviewed changes

efiop merged commit 34ed545 into treeverse:master Dec 30, 2020

mrstegeman deleted the show-origin branch December 30, 2020 22:40

efiop mentioned this pull request Dec 30, 2020

config: temporarily forbid --show-origin without level #5184

Merged

2 tasks

mrstegeman mentioned this pull request Dec 31, 2020

cli: config: show merged config with --show-origin #5188

Merged

2 tasks

Conversation

mrstegeman commented Dec 17, 2020

Uh oh!

shcheklein commented Dec 17, 2020

Uh oh!

mrstegeman commented Dec 17, 2020

Uh oh!

shcheklein commented Dec 18, 2020

Uh oh!

Uh oh!

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

mrstegeman commented Dec 18, 2020

Uh oh!

efiop commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrstegeman commented Dec 22, 2020

Uh oh!

mrstegeman commented Dec 22, 2020

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karajan1001 Dec 29, 2020

Choose a reason for hiding this comment

Uh oh!

mrstegeman Dec 30, 2020

Choose a reason for hiding this comment

Uh oh!

karajan1001 commented Dec 29, 2020

Uh oh!

efiop commented Dec 29, 2020

Uh oh!

mrstegeman commented Dec 30, 2020

Uh oh!

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

efiop commented Dec 30, 2020

Uh oh!

mrstegeman commented Dec 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

efiop left a comment •

edited

Loading

efiop commented Dec 18, 2020 •

edited

Loading