From 1aeab8a7d4dc04b8177bd54914f17128cb941f0a Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 29 Jan 2024 17:03:57 -0700 Subject: [PATCH 1/2] Static defaultExclusions rules were not congruent The default rule set did not reflect the necessary "negationsAfter" state for rules that appear before the default negation rule. This caused Pack to skip the entire ".terraform/modules" directory when it was encountered because it believed the rule to be dominant. Any time a .terraformignore file containing a negation was evaluated, these default rules would be corrected by the parsing algorithm. This is why this bug was not caught by the tests. So I added another Pack test that relies only on the default rules. --- internal/ignorefiles/terraformignore.go | 12 +++---- slug_test.go | 33 +++++++++++++++++++ .../.terraform/modules/README | 1 + .../.terraform/modules/subdir/README | 1 + .../.terraform/plugins/foo.txt | 1 + testdata/archive-dir-defaults-only/bar.txt | 1 + 6 files changed, 43 insertions(+), 6 deletions(-) create mode 100644 testdata/archive-dir-defaults-only/.terraform/modules/README create mode 100644 testdata/archive-dir-defaults-only/.terraform/modules/subdir/README create mode 100644 testdata/archive-dir-defaults-only/.terraform/plugins/foo.txt create mode 100644 testdata/archive-dir-defaults-only/bar.txt diff --git a/internal/ignorefiles/terraformignore.go b/internal/ignorefiles/terraformignore.go index 0ed3ac7..978d6de 100644 --- a/internal/ignorefiles/terraformignore.go +++ b/internal/ignorefiles/terraformignore.go @@ -58,7 +58,6 @@ func readRules(input io.Reader) ([]rule, error) { pattern = "**" + string(os.PathSeparator) + pattern } rule.val = pattern - rule.dirs = strings.Split(pattern, string(os.PathSeparator)) rules = append(rules, rule) currentRuleIndex += 1 } @@ -73,7 +72,6 @@ type rule struct { val string // the value of the rule itself negated bool // prefixed by !, a negated rule negationsAfter bool // negatied rules appear after this rule - dirs []string // directories of the rule regex *regexp.Regexp // regular expression to match for the rule } @@ -173,12 +171,14 @@ func (r *rule) compile() error { var defaultExclusions = []rule{ { - val: strings.Join([]string{"**", ".git", "**"}, string(os.PathSeparator)), - negated: false, + val: strings.Join([]string{"**", ".git", "**"}, string(os.PathSeparator)), + negated: false, + negationsAfter: true, }, { - val: strings.Join([]string{"**", ".terraform", "**"}, string(os.PathSeparator)), - negated: false, + val: strings.Join([]string{"**", ".terraform", "**"}, string(os.PathSeparator)), + negated: false, + negationsAfter: true, }, { val: strings.Join([]string{"**", ".terraform", "modules", "**"}, string(os.PathSeparator)), diff --git a/slug_test.go b/slug_test.go index 3abbc03..c5eac8b 100644 --- a/slug_test.go +++ b/slug_test.go @@ -33,6 +33,39 @@ func TestPack(t *testing.T) { assertArchiveFixture(t, slug, meta) } +func TestPack_defaultRulesOnly(t *testing.T) { + slug := bytes.NewBuffer(nil) + meta, err := Pack("testdata/archive-dir-defaults-only", slug, true) + if err != nil { + t.Fatalf("err: %v", err) + } + + // Make sure .terraform/modules/** are included. + subModuleDir := false + + // Make sure .terraform/plugins are excluded. + pluginsDir := false + + for _, file := range meta.Files { + if strings.HasPrefix(file, filepath.Clean(".terraform/modules/subdir/README")) { + subModuleDir = true + continue + } + + if strings.HasPrefix(file, filepath.Clean(".terraform/plugins/")) { + pluginsDir = true + continue + } + } + if !subModuleDir { + t.Fatal("expected to include .terraform/modules/subdir/README") + } + + if pluginsDir { + t.Fatal("expected to exclude .terraform/plugins") + } +} + func TestPack_rootIsSymlink(t *testing.T) { for _, path := range []string{ "testdata/archive-dir", diff --git a/testdata/archive-dir-defaults-only/.terraform/modules/README b/testdata/archive-dir-defaults-only/.terraform/modules/README new file mode 100644 index 0000000..bc16944 --- /dev/null +++ b/testdata/archive-dir-defaults-only/.terraform/modules/README @@ -0,0 +1 @@ +Keep this file and directory here to test if its properly ignored diff --git a/testdata/archive-dir-defaults-only/.terraform/modules/subdir/README b/testdata/archive-dir-defaults-only/.terraform/modules/subdir/README new file mode 100644 index 0000000..bc16944 --- /dev/null +++ b/testdata/archive-dir-defaults-only/.terraform/modules/subdir/README @@ -0,0 +1 @@ +Keep this file and directory here to test if its properly ignored diff --git a/testdata/archive-dir-defaults-only/.terraform/plugins/foo.txt b/testdata/archive-dir-defaults-only/.terraform/plugins/foo.txt new file mode 100644 index 0000000..c2d32e1 --- /dev/null +++ b/testdata/archive-dir-defaults-only/.terraform/plugins/foo.txt @@ -0,0 +1 @@ +This file should be ignored diff --git a/testdata/archive-dir-defaults-only/bar.txt b/testdata/archive-dir-defaults-only/bar.txt new file mode 100644 index 0000000..5716ca5 --- /dev/null +++ b/testdata/archive-dir-defaults-only/bar.txt @@ -0,0 +1 @@ +bar From c37bb9b4effc317d2c52863ec7ed4a549a492245 Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Mon, 29 Jan 2024 17:10:22 -0700 Subject: [PATCH 2/2] Optimize order of default rules By moving the .git/ exclusion rule to the end of the default list, it's possible to skip the entire .git/ directory walk because no exclusions appear after it. This should slightly speed up packing performance when the default rules are used or when no additional exclusion rules are set by .terraformignore. --- internal/ignorefiles/terraformignore.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/ignorefiles/terraformignore.go b/internal/ignorefiles/terraformignore.go index 978d6de..df7d569 100644 --- a/internal/ignorefiles/terraformignore.go +++ b/internal/ignorefiles/terraformignore.go @@ -171,18 +171,20 @@ func (r *rule) compile() error { var defaultExclusions = []rule{ { - val: strings.Join([]string{"**", ".git", "**"}, string(os.PathSeparator)), + val: strings.Join([]string{"**", ".terraform", "**"}, string(os.PathSeparator)), negated: false, negationsAfter: true, }, + // Place negation rules as high as possible in the list { - val: strings.Join([]string{"**", ".terraform", "**"}, string(os.PathSeparator)), - negated: false, - negationsAfter: true, + val: strings.Join([]string{"**", ".terraform", "modules", "**"}, string(os.PathSeparator)), + negated: true, + negationsAfter: false, }, { - val: strings.Join([]string{"**", ".terraform", "modules", "**"}, string(os.PathSeparator)), - negated: true, + val: strings.Join([]string{"**", ".git", "**"}, string(os.PathSeparator)), + negated: false, + negationsAfter: false, }, }