feat: add SortOptions support for kustomize resource ordering#189
Conversation
Add ability to set kustomize's sortOptions.order (e.g. "fifo") to control resource ordering. This reduces diff noise when applying patches by preserving the original resource order instead of using kustomize's default "legacy" sorting. Adds SortOptions to ChartifyOpts, KustomizeBuildOpts, KustomizeOpts, and PatchOpts. The sortOptions is written to the generated kustomization.yaml in both kustomize-based and patch-based code paths. Also fixes a pre-existing nilness lint warning (tautological condition). Closes #188 Signed-off-by: yxxhero <aiopsclub@163.com>
There was a problem hiding this comment.
Pull request overview
Adds a new SortOptions type (with Order) and threads it from ChartifyOpts through KustomizeBuildOpts and PatchOpts so users can control kustomize's sortOptions.order (e.g. fifo) in the generated kustomization.yaml. Resolves issue #188 to reduce diff noise from kustomize's default legacy resource ordering. Also includes a small drive-by lint fix in chartify.go and updates hardcoded hash expectations in tempdir_test.go (necessary because adding a field to ChartifyOpts changes the spew-based hash).
Changes:
- Introduce
SortOptionsstruct and add it toChartifyOpts,KustomizeBuildOpts,KustomizeOpts, andPatchOpts. - Append
sortOptions:to the generatedkustomization.yamlin bothKustomizeBuildandPatchcode paths when set. - Remove tautological
err != nilcheck increateDirForFileand refresh expected hashes inTestGenerateID.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| chartify.go | Adds SortOptions field on ChartifyOpts and forwards it into KustomizeBuildOpts / PatchOpts; cleans up tautological nil check. |
| kustomize.go | Defines SortOptions, adds it to KustomizeOpts and KustomizeBuildOpts, and appends serialized sortOptions to kustomization.yaml after kustomize edits. |
| patch.go | Adds SortOptions to PatchOpts and appends it to the inline kustomization.yaml content. |
| tempdir_test.go | Updates expected GenerateID hashes to reflect the new ChartifyOpts.SortOptions field affecting the spew-based hash. |
| if err != nil { | ||
| return "", fmt.Errorf("marshaling sortOptions: %w", err) | ||
| } | ||
| f, err := os.ReadFile(kustomizationPath) |
| // SortOptions configures kustomize's sortOptions for resource ordering. | ||
| // Use &SortOptions{Order: "fifo"} to preserve resource order and minimize diff noise. | ||
| // See https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/sortoptions/ | ||
| SortOptions *SortOptions |
| if u.SortOptions != nil { | ||
| sortOptsBytes, err := yaml.Marshal(map[string]*SortOptions{"sortOptions": u.SortOptions}) | ||
| if err != nil { | ||
| return fmt.Errorf("marshaling sortOptions: %w", err) | ||
| } | ||
| kustomizationYamlContent += string(sortOptsBytes) | ||
| } |
- Extract duplicated sortOptions marshaling into marshalSortOptions helper used by both KustomizeBuild and Patch (review comment) - Use r.ReadFile instead of os.ReadFile for consistency with the Runner abstraction (review comment) - Add unit tests for marshalSortOptions and SortOptions behavior in generated kustomization.yaml (review comment) Signed-off-by: yxxhero <aiopsclub@163.com>
|
Addressed all three review comments in b78952e:
|
| t.Run("nil returns no sortOptions content", func(t *testing.T) { | ||
| got, err := marshalSortOptions(nil) | ||
| require.NoError(t, err) | ||
| result := string(got) | ||
| require.Contains(t, result, "sortOptions: null\n") | ||
| }) |
| func TestKustomizationYamlWithSortOptions(t *testing.T) { | ||
| t.Run("Patch generates sortOptions in kustomization.yaml", func(t *testing.T) { | ||
| opts := &SortOptions{Order: "fifo"} | ||
| sortOptsBytes, err := marshalSortOptions(opts) | ||
| require.NoError(t, err) | ||
|
|
||
| kustomizationYamlContent := `kind: "" | ||
| apiversion: "" | ||
| resources: | ||
| - templates/deployment.yaml | ||
| patches: | ||
| - path: strategicmergepatches/patch.0.yaml | ||
| ` | ||
| kustomizationYamlContent += string(sortOptsBytes) | ||
|
|
||
| require.True(t, strings.Contains(kustomizationYamlContent, "sortOptions:")) | ||
| require.True(t, strings.Contains(kustomizationYamlContent, "order: fifo")) | ||
| }) | ||
|
|
||
| t.Run("Patch without SortOptions has no sortOptions in kustomization.yaml", func(t *testing.T) { | ||
| kustomizationYamlContent := `kind: "" | ||
| apiversion: "" | ||
| resources: | ||
| - templates/deployment.yaml | ||
| patches: | ||
| - path: strategicmergepatches/patch.0.yaml | ||
| ` | ||
|
|
||
| require.False(t, strings.Contains(kustomizationYamlContent, "sortOptions:")) | ||
| }) |
|
|
||
| t.Run("KustomizeBuild merges SortOptions from KustomizeBuildOpts", func(t *testing.T) { | ||
| u := &KustomizeBuildOpts{ | ||
| SortOptions: &SortOptions{Order: "fifo"}, | ||
| } | ||
| kustomizeOpts := KustomizeOpts{} | ||
| if u.SortOptions != nil { | ||
| kustomizeOpts.SortOptions = u.SortOptions | ||
| } | ||
| require.NotNil(t, kustomizeOpts.SortOptions) | ||
| require.Equal(t, "fifo", kustomizeOpts.SortOptions.Order) | ||
| }) | ||
|
|
||
| t.Run("KustomizeBuild without SortOptions leaves kustomizeOpts.SortOptions nil", func(t *testing.T) { | ||
| u := &KustomizeBuildOpts{} | ||
| kustomizeOpts := KustomizeOpts{} | ||
| if u.SortOptions != nil { | ||
| kustomizeOpts.SortOptions = u.SortOptions | ||
| } | ||
| require.Nil(t, kustomizeOpts.SortOptions) | ||
| }) |
| if kustomizeOpts.SortOptions != nil { | ||
| sortOptsBytes, err := marshalSortOptions(kustomizeOpts.SortOptions) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| f, err := r.ReadFile(kustomizationPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("reading kustomization.yaml for sortOptions: %w", err) | ||
| } | ||
| if err := r.WriteFile(kustomizationPath, append(f, sortOptsBytes...), 0644); err != nil { | ||
| return "", err | ||
| } | ||
| } |
| type SortOptions struct { | ||
| Order string `yaml:"order"` | ||
| } |
- Guard marshalSortOptions(nil) to return nil/empty instead of 'sortOptions: null' (review comment) - Add validate() on SortOptions.Order to reject invalid values early, accepted values: legacy, fifo (review comment) - Call validation from Chartify() before any kustomize operations - Rewrite tests to exercise real Runner.Patch code path via WriteFile capture, instead of inline string assertions (review comment) - Add comment explaining sortOptions uses byte append because kustomize has no 'edit set sortoptions' command (review comment) - Fix PR description to only list accepted values: legacy, fifo Signed-off-by: yxxhero <aiopsclub@163.com>
|
Addressed all second-round review comments in 65d008a:
|
Summary
SortOptionstype withOrderfield to control kustomize's resource sorting order (accepted values:"legacy","fifo")SortOptions.Orderearly inChartify()to reject invalid values with a clear errorSortOptionsthroughChartifyOpts→KustomizeBuildOpts(kustomize-based apps) andPatchOpts(patch-based builds)sortOptionsto the generatedkustomization.yamlin both code paths (byte append, since kustomize has noedit set sortoptionscommand)tautological condition: non-nil != nil)Usage
This generates:
Closes #188