dvcignore: read ignore patterns from global .dvcignore by meshde · Pull Request #8280 · treeverse/dvc

meshde · 2022-09-11T03:48:09Z

Read the patterns from system/global config level ignore file and include these as part of
the default ignore pattern used while initiating the ignore trie

Fixes #8261

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

Read the patterns from system-level .dvcignore and include these as part of the default ignore pattern used while initiating the ignore trie Fixes treeverse#8261

for more information, see https://pre-commit.ci

dtrifiro · 2022-09-12T16:03:50Z


        key = self._get_key(root_dir)
+
+        sys_root_dvcignore_path = self.fs.path.join(


Hey! Thanks for contributing 🚀

Just as a small note: git's behaviour for global gitignore files, is to look for a global gitignore in $XDG_CONFIG_HOME/git/ignore.

For dvc user-level config, we use appdirs.user_config_dir (system_config_dir for system-level):
https://github.com/iterative/dvc/blob/8e5f4a8f35a15e741031bf1c9dbe4728fa55021d/dvc/config.py#L107-L116

So it could make sense to keep the user's dvcignore in appdirs.user_config_dir(cls.APPNAME, cls.APPAUTHOR) (note: no leading .)

@dtrifiro Thank you for your review!

Happy to make that change but IMHO ~/.dvcignore is more intuitive to maintain than ~/Library/Application Support/dvc/dvcignore (macOS).

I understand that the global config options get stored in ~/Library/Application Support/dvc/config but something to keep in mind is that the documented way of setting and reading global config options is through the command dvc config --global rather than maintaining the config file itself.

Lmk what you think, I'm happy to make it go either way.

IMO it would be better to handle this the way that git does - you tell git where to look for the file via the core.excludesfile config option.

We should be adding a similar config option in DVC, so the user can do dvc config --global core.excludesfile $HOME/.dvcignore if that's where they want it to go. If the user has not set the option, it should default to looking in the proper DVC user config directories per-platform (~/Library/Application Support on macos, XDG config dirs on linux, and %localappdata% on windows).

Also note that by default we need to check the global (user) level config location first. But we should also check the system level location as well if there is no global level .dvcignore.

@pmrowla okay cool thanks. I'll update the code accordingly!

karajan1001 · 2022-09-13T07:48:52Z

LGTM, except for the question discussed above.
Still need an update on the doc side.

for more information, see https://pre-commit.ci

meshde · 2022-09-15T09:38:39Z

Still need an update on the doc side.

@karajan1001 thank you for the review!

Just raised a PR to update the docs here: treeverse/dvc.org#3950

meshde · 2022-09-15T09:39:48Z

@pmrowla @dtrifiro just updated the code based on your comments.

lmk what you think of these changes.

pmrowla · 2022-09-16T05:38:14Z

+@pytest.fixture(autouse=True)
+def tmp_config_dir(mocker, tmp_path):
+    """
+    Fixture to prevent modifying/reading the actual global config
+    """
+
+    for level in ["global", "system"]:
+        os.makedirs(_get_tmp_config_dir(tmp_path, level))
+
+    def get_tmp_config_dir(level):
+        return str(_get_tmp_config_dir(tmp_path, level))
+
+    mocker.patch("dvc.config.Config.get_dir", side_effect=get_tmp_config_dir)


This shouldn't be needed, we already isolate the environment in the isolate fixture from tests/conftest.py.

So when I was writing the tests, the .dvcignore from my system dir (/Library/Application Support/dvc/.dvcignore) was being read in the tests, which is why I ended up mocking Config.get_dir.

Now that I look into it further, the isolate fixture does return an isolated directory for Config.get_dir("global") but does not seem to do so for Config.get_dir("system") and returns /Library/Application Support/dvc instead.

I'll see if I can make the isloate fixture also isolate the system config directory.

So it turns out that the isolate fixture in it's current state is not completely ready to handle global/system dvc configs. The PR that added the this fixture mentions this fact in a comment: #6857 (comment).

Got it to isolate system and global dvc config directories and tried to cover all cases in the following way:
https://github.com/iterative/dvc/pull/8280/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128

There were some situations where manipulating env vars was not helpful. In such cases, this PR mocks the functions from appdirs instead.

Lmk what you think of this

for more information, see https://pre-commit.ci

dtrifiro

See comments above

…atterns

meshde · 2022-09-30T03:48:22Z

@dtrifiro updated the PR as per your suggestions.

Lmk what you think now.

efiop · 2022-10-05T10:43:59Z

        self.root_dir = root_dir
        self.ignores_trie_fs = Trie()
        self._ignores_trie_subrepos = Trie()
+        self.config = Config()


This seems to be breaking the scope. Can't we just pass particular options when creating dvcignore in the repo?

Btw, doing that will allow simplifying tests by not having to monkeypatch global config dir all the time.

shcheklein · 2022-12-21T17:44:32Z

@meshde @iterative/dvc any progress here folks?

efiop · 2023-01-09T00:10:47Z

Closing as stale. Please feel free to reopen if you'll find time to address #8280 (comment) This change is useful, but we don't have the capacity to take it over ourselves right now.

meshde and others added 3 commits September 11, 2022 09:12

dvcignore: read ignore patterns from system level .dvcignore

f5d68c1

Read the patterns from system-level .dvcignore and include these as part of the default ignore pattern used while initiating the ignore trie Fixes treeverse#8261

[pre-commit.ci] auto fixes from pre-commit.com hooks

fb4bfff

for more information, see https://pre-commit.ci

fix lint

730a755

meshde marked this pull request as ready for review September 11, 2022 05:40

meshde mentioned this pull request Sep 11, 2022

feature: System-level .dvcignore #8261

Closed

karajan1001 self-requested a review September 11, 2022 12:00

dtrifiro reviewed Sep 12, 2022

View reviewed changes

dtrifiro self-requested a review September 12, 2022 16:04

pmrowla changed the title ~~dvcignore: read ignore patterns from system level .dvcignore~~ dvcignore: read ignore patterns from global .dvcignore Sep 13, 2022

meshde mentioned this pull request Sep 15, 2022

dvcignore: add docs related to global dvcignore treeverse/dvc.org#3950

Closed

meshde and others added 3 commits September 15, 2022 15:06

dvcignore: allow reading patterns from core.excludesfile

277c7bc

Merge branch 'main' into system-dvcignore

112ad4f

[pre-commit.ci] auto fixes from pre-commit.com hooks

759d427

for more information, see https://pre-commit.ci

meshde requested review from dtrifiro and pmrowla and removed request for dtrifiro, karajan1001 and pmrowla September 15, 2022 09:39

meshde added 4 commits September 15, 2022 15:30

fix lint

7f5500e

fix lint

5a073cb

Merge branch 'main' into system-dvcignore

e0b85d6

Merge branch 'main' into system-dvcignore

3632af4

pmrowla reviewed Sep 16, 2022

View reviewed changes

meshde and others added 4 commits September 17, 2022 16:37

tests: make isolate fixture isolate system-level config dir

3bc410d

[pre-commit.ci] auto fixes from pre-commit.com hooks

69a74b6

for more information, see https://pre-commit.ci

tests: do not fail if dir exists while creation

6573f21

tests: print vars to debug ci check failure

ddff2f0

meshde and others added 8 commits September 18, 2022 07:28

tests: print /Users/meshde to debug ci check

bfe9b85

tests: remove prints

d02df22

tests: set XDG_CONFIG_HOME in isolate fixture

4794cb5

tests: support global and system dvc config isolation

22cd0c2

tests: use a custom file name for excludesfile

2790455

tests: allow config dirs to be called for different apps

d0cbaf7

[pre-commit.ci] auto fixes from pre-commit.com hooks

d01f182

for more information, see https://pre-commit.ci

Merge branch 'main' into system-dvcignore

f043e95

This comment was marked as outdated.

Sign in to view

meshde requested review from pmrowla and removed request for dtrifiro September 18, 2022 16:11

meshde added 2 commits September 19, 2022 15:41

Merge branch 'main' into system-dvcignore

83f55e4

Merge branch 'main' into system-dvcignore

a62a1a1

dtrifiro reviewed Sep 27, 2022

View reviewed changes

Comment thread dvc/ignore.py Outdated

dtrifiro reviewed Sep 27, 2022

View reviewed changes

Comment thread dvc/ignore.py Outdated

dtrifiro suggested changes Sep 27, 2022

View reviewed changes

meshde added 3 commits September 30, 2022 07:23

Merge branch 'main' into system-dvcignore

3ec4f60

dvcignore: refactor to have class method that returns global ignore p…

aa1c11d

…atterns

dvcignore: use 'ignore' as the global/system ignore file name

eae144f

meshde requested review from dtrifiro and removed request for pmrowla September 30, 2022 03:46

Merge branch 'main' into system-dvcignore

f7b61d1

efiop requested a review from karajan1001 October 5, 2022 10:40

efiop reviewed Oct 5, 2022

View reviewed changes

efiop closed this Jan 9, 2023


		key = self._get_key(root_dir)

		sys_root_dvcignore_path = self.fs.path.join(

Conversation

meshde commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtrifiro Sep 12, 2022 • 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!

meshde Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

pmrowla Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

pmrowla Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

meshde Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

karajan1001 commented Sep 13, 2022

Uh oh!

meshde commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshde commented Sep 15, 2022

Uh oh!

pmrowla Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

meshde Sep 16, 2022 • 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!

meshde Sep 18, 2022 • 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!

This comment was marked as outdated.

Uh oh!

Uh oh!

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

meshde commented Sep 30, 2022

Uh oh!

efiop Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

efiop Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

shcheklein commented Dec 21, 2022

Uh oh!

efiop commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

meshde commented Sep 11, 2022 •

edited

Loading

dtrifiro Sep 12, 2022 •

edited

Loading

meshde commented Sep 15, 2022 •

edited

Loading

meshde Sep 16, 2022 •

edited

Loading

meshde Sep 18, 2022 •

edited

Loading

dtrifiro left a comment •

edited

Loading