From f77974626cab760f49d6c7b32d5efda7cc4bb8a4 Mon Sep 17 00:00:00 2001 From: dev-milos Date: Thu, 13 Aug 2026 14:33:59 +0200 Subject: [PATCH 1/3] compare: treat an empty value and an absent key as equal The importer stopped writing description and homepage_url when the value is empty, so every existing config differs from a fresh import of the same unchanged repository. compare exists to answer whether something changed, and it started answering wrongly for half the fleet. Three features filter through it and all three degraded: import and bulk-import stopped dropping unchanged repositories from their pull requests, and drift-check began reporting repositories that had not drifted. Normalising in compare rather than reverting the importer keeps the cleaner generated output and covers the whole class, so a future serialization change that moves a field between empty and absent stops mattering. The pass is deliberately narrow. Only empty string and null scalars are dropped; false and 0 are meaningful and stay, which the YAML tag distinguishes. Containers left empty are kept, since collapsing them asserts a broader equivalence than this needs. --- .../pkg/compare/compare.go | 34 +++++++++++++++++++ .../pkg/compare/compare_test.go | 12 +++++++ .../compare/testdata/existing/existing5.yaml | 21 ++++++++++++ .../compare/testdata/existing/existing6.yaml | 8 +++++ .../compare/testdata/imported/imported5.yaml | 18 ++++++++++ .../compare/testdata/imported/imported6.yaml | 7 ++++ 6 files changed, 100 insertions(+) create mode 100644 feature/github-repo-importer/pkg/compare/testdata/existing/existing5.yaml create mode 100644 feature/github-repo-importer/pkg/compare/testdata/existing/existing6.yaml create mode 100644 feature/github-repo-importer/pkg/compare/testdata/imported/imported5.yaml create mode 100644 feature/github-repo-importer/pkg/compare/testdata/imported/imported6.yaml diff --git a/feature/github-repo-importer/pkg/compare/compare.go b/feature/github-repo-importer/pkg/compare/compare.go index aa97254..57cec4c 100644 --- a/feature/github-repo-importer/pkg/compare/compare.go +++ b/feature/github-repo-importer/pkg/compare/compare.go @@ -102,6 +102,7 @@ func hashNormalizedYamlFile(path string) (string, error) { removeKey(root, "id") removeKey(root, "branch_policy_ids") removeKey(root, "tag_policy_ids") + removeEmptyValues(root) sortMappingNode(root) } @@ -136,6 +137,39 @@ func removeKey(node *yaml.Node, target string) { } } +// removeEmptyValues drops mapping entries whose value is an empty string or null, so a +// config that writes `homepage_url: ""` and one that omits the key entirely hash the same. +// Only scalars qualify: false and 0 are meaningful values and are kept. Containers left +// empty by this pass are also kept, since collapsing them asserts an equivalence this does +// not need. +func removeEmptyValues(node *yaml.Node) { + switch node.Kind { + case yaml.MappingNode: + newContent := make([]*yaml.Node, 0, len(node.Content)) + for i := 0; i < len(node.Content); i += 2 { + k := node.Content[i] + v := node.Content[i+1] + if isEmptyScalar(v) { + continue + } + removeEmptyValues(v) + newContent = append(newContent, k, v) + } + node.Content = newContent + case yaml.SequenceNode: + for _, elem := range node.Content { + removeEmptyValues(elem) + } + } +} + +func isEmptyScalar(node *yaml.Node) bool { + if node.Kind != yaml.ScalarNode { + return false + } + return node.Tag == "!!null" || (node.Tag == "!!str" && node.Value == "") +} + func sortMappingNode(node *yaml.Node) { if node.Kind != yaml.MappingNode { return diff --git a/feature/github-repo-importer/pkg/compare/compare_test.go b/feature/github-repo-importer/pkg/compare/compare_test.go index 077c922..befd8f4 100644 --- a/feature/github-repo-importer/pkg/compare/compare_test.go +++ b/feature/github-repo-importer/pkg/compare/compare_test.go @@ -37,6 +37,18 @@ func TestHashingYamlFiles(t *testing.T) { pathOfFresh: "testdata/existing/existing4.yaml", wantEqual: true, }, + { + name: "empty and null values are equivalent to the key being absent, while false is kept", + pathOfImported: "testdata/imported/imported5.yaml", + pathOfFresh: "testdata/existing/existing5.yaml", + wantEqual: true, + }, + { + name: "a real difference is still detected alongside an empty value", + pathOfImported: "testdata/imported/imported6.yaml", + pathOfFresh: "testdata/existing/existing6.yaml", + wantEqual: false, + }, } for _, tt := range tests { diff --git a/feature/github-repo-importer/pkg/compare/testdata/existing/existing5.yaml b/feature/github-repo-importer/pkg/compare/testdata/existing/existing5.yaml new file mode 100644 index 0000000..044bdfc --- /dev/null +++ b/feature/github-repo-importer/pkg/compare/testdata/existing/existing5.yaml @@ -0,0 +1,21 @@ +description: "" +homepage_url: "" +visibility: public +default_branch: main +has_issues: true +has_downloads: false +allow_auto_merge: false +archived: false +license_template: +rulesets: + - enforcement: active + name: test + rules: + deletion: true + non_fast_forward: true + target: branch + conditions: + ref_name: + include: + - ~DEFAULT_BRANCH +vulnerability_alerts_enabled: true diff --git a/feature/github-repo-importer/pkg/compare/testdata/existing/existing6.yaml b/feature/github-repo-importer/pkg/compare/testdata/existing/existing6.yaml new file mode 100644 index 0000000..457bc6c --- /dev/null +++ b/feature/github-repo-importer/pkg/compare/testdata/existing/existing6.yaml @@ -0,0 +1,8 @@ +description: Managed by Terraform +homepage_url: "" +visibility: public +default_branch: main +has_issues: true +has_downloads: false +archived: false +vulnerability_alerts_enabled: true diff --git a/feature/github-repo-importer/pkg/compare/testdata/imported/imported5.yaml b/feature/github-repo-importer/pkg/compare/testdata/imported/imported5.yaml new file mode 100644 index 0000000..7192bad --- /dev/null +++ b/feature/github-repo-importer/pkg/compare/testdata/imported/imported5.yaml @@ -0,0 +1,18 @@ +visibility: public +default_branch: main +has_issues: true +has_downloads: false +allow_auto_merge: false +archived: false +rulesets: + - enforcement: active + name: test + rules: + deletion: true + non_fast_forward: true + target: branch + conditions: + ref_name: + include: + - ~DEFAULT_BRANCH +vulnerability_alerts_enabled: true diff --git a/feature/github-repo-importer/pkg/compare/testdata/imported/imported6.yaml b/feature/github-repo-importer/pkg/compare/testdata/imported/imported6.yaml new file mode 100644 index 0000000..05186fb --- /dev/null +++ b/feature/github-repo-importer/pkg/compare/testdata/imported/imported6.yaml @@ -0,0 +1,7 @@ +description: Edited by hand outside the tool +visibility: public +default_branch: main +has_issues: true +has_downloads: false +archived: false +vulnerability_alerts_enabled: true From 2c21f9f02f4ea9fb25c195964c0694098b261251 Mon Sep 17 00:00:00 2001 From: dev-milos Date: Thu, 13 Aug 2026 15:15:37 +0200 Subject: [PATCH 2/3] drop the explanatory comment on removeEmptyValues --- feature/github-repo-importer/pkg/compare/compare.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/feature/github-repo-importer/pkg/compare/compare.go b/feature/github-repo-importer/pkg/compare/compare.go index 57cec4c..92679c2 100644 --- a/feature/github-repo-importer/pkg/compare/compare.go +++ b/feature/github-repo-importer/pkg/compare/compare.go @@ -137,11 +137,6 @@ func removeKey(node *yaml.Node, target string) { } } -// removeEmptyValues drops mapping entries whose value is an empty string or null, so a -// config that writes `homepage_url: ""` and one that omits the key entirely hash the same. -// Only scalars qualify: false and 0 are meaningful values and are kept. Containers left -// empty by this pass are also kept, since collapsing them asserts an equivalence this does -// not need. func removeEmptyValues(node *yaml.Node) { switch node.Kind { case yaml.MappingNode: From 831a2f564e2de2f0b29537919757ad0d130a21ac Mon Sep 17 00:00:00 2001 From: dev-milos Date: Thu, 13 Aug 2026 15:56:55 +0200 Subject: [PATCH 3/3] test: cover a false value present on one side only The falsey keys in the empty-versus-absent case sat on both sides, so stripping them would have stripped both and the case would still have passed. The new pair has the falsey key on one side only and asserts inequality, which fails if false is ever treated as empty. --- feature/github-repo-importer/pkg/compare/compare_test.go | 8 +++++++- .../pkg/compare/testdata/existing/existing7.yaml | 6 ++++++ .../pkg/compare/testdata/imported/imported7.yaml | 5 +++++ 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 feature/github-repo-importer/pkg/compare/testdata/existing/existing7.yaml create mode 100644 feature/github-repo-importer/pkg/compare/testdata/imported/imported7.yaml diff --git a/feature/github-repo-importer/pkg/compare/compare_test.go b/feature/github-repo-importer/pkg/compare/compare_test.go index befd8f4..af2bc48 100644 --- a/feature/github-repo-importer/pkg/compare/compare_test.go +++ b/feature/github-repo-importer/pkg/compare/compare_test.go @@ -38,7 +38,7 @@ func TestHashingYamlFiles(t *testing.T) { wantEqual: true, }, { - name: "empty and null values are equivalent to the key being absent, while false is kept", + name: "empty and null values are equivalent to the key being absent", pathOfImported: "testdata/imported/imported5.yaml", pathOfFresh: "testdata/existing/existing5.yaml", wantEqual: true, @@ -49,6 +49,12 @@ func TestHashingYamlFiles(t *testing.T) { pathOfFresh: "testdata/existing/existing6.yaml", wantEqual: false, }, + { + name: "a false value on one side only is a difference, not an empty value", + pathOfImported: "testdata/imported/imported7.yaml", + pathOfFresh: "testdata/existing/existing7.yaml", + wantEqual: false, + }, } for _, tt := range tests { diff --git a/feature/github-repo-importer/pkg/compare/testdata/existing/existing7.yaml b/feature/github-repo-importer/pkg/compare/testdata/existing/existing7.yaml new file mode 100644 index 0000000..4be3d6d --- /dev/null +++ b/feature/github-repo-importer/pkg/compare/testdata/existing/existing7.yaml @@ -0,0 +1,6 @@ +visibility: public +default_branch: main +has_issues: true +has_downloads: false +archived: false +vulnerability_alerts_enabled: true diff --git a/feature/github-repo-importer/pkg/compare/testdata/imported/imported7.yaml b/feature/github-repo-importer/pkg/compare/testdata/imported/imported7.yaml new file mode 100644 index 0000000..d66001b --- /dev/null +++ b/feature/github-repo-importer/pkg/compare/testdata/imported/imported7.yaml @@ -0,0 +1,5 @@ +visibility: public +default_branch: main +has_issues: true +archived: false +vulnerability_alerts_enabled: true