diff --git a/.github/renovate.json5 b/.github/renovate.json5 index 99951e9..0cf2376 100644 --- a/.github/renovate.json5 +++ b/.github/renovate.json5 @@ -139,7 +139,8 @@ ], datasourceTemplate: 'github-releases', depNameTemplate: 'github/codeql-action', - extractVersionTemplate: '^codeql-bundle-(?.*)$', + extractVersionTemplate: '^codeql-bundle-v(?.*)$', + versioning: 'semver', }, ], // Renovate doesn't have native Earthfile support, but because Earthfile diff --git a/.golangci.yml b/.golangci.yml index a2e17f9..7627a7e 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,21 +1,12 @@ -run: - timeout: 10m - +version: "2" output: - # colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number" formats: - - format: colored-line-number + text: path: stderr - + colors: true linters: - enable-all: true - fast: false - + default: all disable: - # These linters are all deprecated. We disable them explicitly to avoid the - # linter logging deprecation warnings. - - tenv - # These are linters we'd like to enable, but that will be labor intensive to # make existing code compliant. - wrapcheck @@ -30,7 +21,7 @@ linters: # These linters add whitespace in an attempt to make code more readable. # This isn't a widely accepted Go best practice, and would be laborious to # apply to existing code. - - wsl + - wsl - nlreturn # Warns about uses of fmt.Sprintf that are less performant than alternatives @@ -82,7 +73,7 @@ linters: # Warns about returning interfaces rather than concrete types. We do think # it's best to avoid returning interfaces where possible. However, at the # time of writing enabling this linter would only catch the (many) cases - # where we must return an interface. + # where we must return an interface. - ireturn # Warns about returning named variables. We do think it's best to avoid @@ -93,208 +84,176 @@ linters: # to communicate what the bool means. - nonamedreturns - # Warns about using magic numbers. We do think it's best to avoid magic # numbers, but we should not be strict about it. - mnd -linters-settings: - errcheck: - # report about not checking of errors in type assetions: `a := b.(MyStruct)`; - # default is false: such cases aren't reported by default. - check-type-assertions: false - - # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`; - # default is false: such cases aren't reported by default. - check-blank: false - - govet: - # report about shadowed variables - disable: - - shadow - - gofmt: - # simplify code: gofmt with `-s` option, true by default - simplify: true - - gci: - custom-order: true - sections: - - standard - - default - - prefix(github.com/crossplane/crossplane-runtime) - - prefix(github.com/crossplane/crossplane) - - blank - - dot - - dupl: - # tokens count to trigger issue, 150 by default - threshold: 100 - - goconst: - # minimal length of string constant, 3 by default - min-len: 3 - # minimal occurrences count to trigger, 3 by default - min-occurrences: 5 - - lll: - # tab width in spaces. Default to 1. - tab-width: 1 - - unused: - # treat code as a program (not a library) and report unused exported identifiers; default is false. - # XXX: if you enable this setting, unused will report a lot of false-positives in text editors: - # if it's called for subdir of a project it can't find funcs usages. All text editor integrations - # with golangci-lint call it on a directory with the changed file. - exported-fields-are-used: true - - unparam: - # Inspect exported functions, default is false. Set to true if no external program/library imports your code. - # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors: - # if it's called for subdir of a project it can't find external interfaces. All text editor integrations - # with golangci-lint call it on a directory with the changed file. - check-exported: false - - nakedret: - # make an issue if func has more lines of code than this setting and it has naked returns; default is 30 - max-func-lines: 30 - - prealloc: - # XXX: we don't recommend using this linter before doing performance profiling. - # For most programs usage of prealloc will be a premature optimization. - - # Report preallocation suggestions only on simple loops that have no returns/breaks/continues/gotos in them. - # True by default. - simple: true - range-loops: true # Report preallocation suggestions on range loops, true by default - for-loops: false # Report preallocation suggestions on for loops, false by default - - gocritic: - # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks. - # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags". - enabled-tags: - - performance - - settings: # settings passed to gocritic - captLocal: # must be valid enabled check name - paramsOnly: true - rangeValCopy: - sizeThreshold: 32 - - nolintlint: - require-explanation: true - require-specific: true - - depguard: - rules: - no_third_party_test_libraries: - list-mode: lax - files: - - $test - deny: - - pkg: github.com/stretchr/testify - desc: "See https://go.dev/wiki/TestComments#assert-libraries" - - pkg: github.com/onsi/ginkgo - desc: "See https://go.dev/wiki/TestComments#assert-libraries" - - pkg: github.com/onsi/gomega - desc: "See https://go.dev/wiki/TestComments#assert-libraries" - - interfacebloat: - max: 5 - - tagliatelle: - case: + # Disabled due to extensive refactoring required and maintenance overhead + - funcorder # Requires reordering methods which adds complexity + - noinlineerr # Inline error handling is readable and common in Go + - noctx # Context requirements can be added incrementally + settings: + depguard: rules: - json: goCamel - + no_third_party_test_libraries: + list-mode: lax + files: + - $test + deny: + - pkg: github.com/stretchr/testify + desc: "See https://go.dev/wiki/TestComments#assert-libraries" + - pkg: github.com/onsi/ginkgo + desc: "See https://go.dev/wiki/TestComments#assert-libraries" + - pkg: github.com/onsi/gomega + desc: "See https://go.dev/wiki/TestComments#assert-libraries" + dupl: + # tokens count to trigger issue, 150 by default + threshold: 100 + errcheck: + # report about not checking of errors in type assetions: `a := b.(MyStruct)`; + # default is false: such cases aren't reported by default. + check-type-assertions: false + # report about assignment of errors to blank identifier: `num, _ := strconv.Atoi(numStr)`; + # default is false: such cases aren't reported by default. + check-blank: false + goconst: + # minimal length of string constant, 3 by default + min-len: 3 + # minimal occurrences count to trigger, 3 by default + min-occurrences: 5 + gocritic: + # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks. + # Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags". + enabled-tags: + - performance + settings: # settings passed to gocritic + captLocal: # must be valid enabled check name + paramsOnly: true + rangeValCopy: + sizeThreshold: 32 + govet: + # report about shadowed variables + disable: + - shadow + interfacebloat: + max: 5 + lll: + # tab width in spaces. Default to 1. + tab-width: 1 + nakedret: + # make an issue if func has more lines of code than this setting and it has naked returns; default is 30 + max-func-lines: 30 + nolintlint: + require-explanation: true + require-specific: true + prealloc: + # XXX: we don't recommend using this linter before doing performance profiling. + # For most programs usage of prealloc will be a premature optimization. + + # Report preallocation suggestions only on simple loops that have no returns/breaks/continues/gotos in them. + # True by default. + simple: true + range-loops: true # Report preallocation suggestions on range loops, true by default + for-loops: false # Report preallocation suggestions on for loops, false by default + tagliatelle: + case: + rules: + json: goCamel + unparam: + # Inspect exported functions, default is false. Set to true if no external program/library imports your code. + # XXX: if you enable this setting, unparam will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find external interfaces. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + check-exported: false + unused: + # treat code as a program (not a library) and report unused exported identifiers; default is false. + # XXX: if you enable this setting, unused will report a lot of false-positives in text editors: + # if it's called for subdir of a project it can't find funcs usages. All text editor integrations + # with golangci-lint call it on a directory with the changed file. + exported-fields-are-used: true + exclusions: + generated: lax + rules: + # Exclude some linters from running on tests files. + - linters: + - containedctx + - errcheck + - forcetypeassert + - gochecknoglobals + - gochecknoinits + - gocognit + - gosec + - scopelint + - unparam + path: _test(ing)?\.go + + # Ease some gocritic warnings on test files. + - linters: + - gocritic + path: _test\.go + text: (unnamedResult|exitAfterDefer) + + # It's idiomatic to register Kubernetes types with a package scoped + # SchemeBuilder using an init function. + - linters: + - gochecknoglobals + - gochecknoinits + path: apis/ + + # These are performance optimisations rather than style issues per se. + # They warn when function arguments or range values copy a lot of memory + # rather than using a pointer. + - linters: + - gocritic + text: '(hugeParam|rangeValCopy):' + + # This "TestMain should call os.Exit to set exit code" warning is not clever + # enough to notice that we call a helper method that calls os.Exit. + - linters: + - staticcheck + text: 'SA3000:' + + # This is a "potential hardcoded credentials" warning. It's triggered by + # any variable with 'secret' in the same, and thus hits a lot of false + # positives in Kubernetes land where a Secret is an object type. + - linters: + - gosec + text: 'G101:' + + # This is an 'errors unhandled' warning that duplicates errcheck. + - linters: + - gosec + text: 'G104:' + + # This is about implicit memory aliasing in a range loop. + # This is a false positive with Go v1.22 and above. + - linters: + - gosec + text: 'G601:' + + # Some k8s dependencies do not have JSON tags on all fields in structs. + - linters: + - musttag + path: k8s.io/ + + # Various fields related to native patch and transform Composition are + # deprecated, but we can't drop support from Crossplane 1.x. We ignore the + # warnings globally instead of suppressing them with comments everywhere. + - linters: + - staticcheck + text: 'SA1019: .+ is deprecated: Use Composition Functions instead.' + paths: + - zz_generated\..+\.go$ + - .+\.pb.go$ + - third_party$ + - builtin$ + - examples$ issues: - # Excluding generated files. - exclude-files: - - "zz_generated\\..+\\.go$" - - ".+\\.pb.go$" - # Excluding configuration per-path and per-linter. - exclude-rules: - # Exclude some linters from running on tests files. - - path: _test(ing)?\.go - linters: - - gocognit - - errcheck - - gosec - - scopelint - - unparam - - gochecknoinits - - gochecknoglobals - - containedctx - - forcetypeassert - - # Ease some gocritic warnings on test files. - - path: _test\.go - text: "(unnamedResult|exitAfterDefer)" - linters: - - gocritic - - # It's idiomatic to register Kubernetes types with a package scoped - # SchemeBuilder using an init function. - - path: apis/ - linters: - - gochecknoinits - - gochecknoglobals - - # These are performance optimisations rather than style issues per se. - # They warn when function arguments or range values copy a lot of memory - # rather than using a pointer. - - text: "(hugeParam|rangeValCopy):" - linters: - - gocritic - - # This "TestMain should call os.Exit to set exit code" warning is not clever - # enough to notice that we call a helper method that calls os.Exit. - - text: "SA3000:" - linters: - - staticcheck - - - text: "k8s.io/api/core/v1" - linters: - - goimports - - # This is a "potential hardcoded credentials" warning. It's triggered by - # any variable with 'secret' in the same, and thus hits a lot of false - # positives in Kubernetes land where a Secret is an object type. - - text: "G101:" - linters: - - gosec - - gas - - # This is an 'errors unhandled' warning that duplicates errcheck. - - text: "G104:" - linters: - - gosec - - gas - - # This is about implicit memory aliasing in a range loop. - # This is a false positive with Go v1.22 and above. - - text: "G601:" - linters: - - gosec - - gas - - # Some k8s dependencies do not have JSON tags on all fields in structs. - - path: k8s.io/ - linters: - - musttag - - # Various fields related to native patch and transform Composition are - # deprecated, but we can't drop support from Crossplane 1.x. We ignore the - # warnings globally instead of suppressing them with comments everywhere. - - text: "SA1019: .+ is deprecated: Use Composition Functions instead." - linters: - - staticcheck + # Maximum issues count per one linter. Set to 0 to disable. Default is 50. + max-issues-per-linter: 0 - # Independently from option `exclude` we use default exclude patterns, - # it can be disabled by this option. To list all - # excluded by default patterns execute `golangci-lint run --help`. - # Default value for this option is true. - exclude-use-default: false + # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. + max-same-issues: 0 # Show only new issues: if there are unstaged changes or untracked files, # only those changes are analyzed, else only changes in HEAD~ are analyzed. @@ -303,9 +262,30 @@ issues: # of integration: much better don't allow issues in new code. # Default is false. new: false - - # Maximum issues count per one linter. Set to 0 to disable. Default is 50. - max-issues-per-linter: 0 - - # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. - max-same-issues: 0 +formatters: + enable: + - gci + - gofmt + - gofumpt + - goimports + settings: + gci: + custom-order: true + sections: + - standard + - default + - prefix(github.com/crossplane/crossplane-runtime) + - prefix(github.com/crossplane/crossplane) + - blank + - dot + gofmt: + # simplify code: gofmt with `-s` option, true by default + simplify: true + exclusions: + generated: lax + paths: + - zz_generated\..+\.go$ + - .+\.pb.go$ + - third_party$ + - builtin$ + - examples$ diff --git a/Earthfile b/Earthfile index 5ec3c4b..b0337da 100644 --- a/Earthfile +++ b/Earthfile @@ -238,7 +238,7 @@ go-test: # go-lint lints Go code. go-lint: - ARG GOLANGCI_LINT_VERSION=v1.64.8 + ARG GOLANGCI_LINT_VERSION=v2.4.0 FROM +go-modules # This cache is private because golangci-lint doesn't support concurrent runs. CACHE --id go-lint --sharing private /root/.cache/golangci-lint diff --git a/cmd/diff/app_context.go b/cmd/diff/app_context.go index ad57f3d..00b4259 100644 --- a/cmd/diff/app_context.go +++ b/cmd/diff/app_context.go @@ -54,7 +54,8 @@ func NewAppContext(config *rest.Config, logger logging.Logger) (*AppContext, err // Initialize initializes all clients. func (a *AppContext) Initialize(ctx context.Context, logger logging.Logger) error { // Initialize Crossplane client - if err := a.XpClients.Initialize(ctx, logger); err != nil { + err := a.XpClients.Initialize(ctx, logger) + if err != nil { return errors.Wrap(err, "cannot initialize Crossplane client") } diff --git a/cmd/diff/client/core/core.go b/cmd/diff/client/core/core.go index 3ac548f..5fd4384 100644 --- a/cmd/diff/client/core/core.go +++ b/cmd/diff/client/core/core.go @@ -38,10 +38,12 @@ func NewClients(config *rest.Config) (*Clients, error) { if err != nil { return nil, errors.Wrap(err, "cannot create dynamic client") } + disClient, err := makeDiscoveryClient(config) if err != nil { return nil, errors.Wrap(err, "cannot create discovery client") } + xrmClient, err := makeXrmClient(config) if err != nil { return nil, errors.Wrap(err, "cannot create xrm client") @@ -74,6 +76,7 @@ func makeXrmClient(config *rest.Config) (*xrm.Client, error) { if err != nil { return nil, errors.Wrap(err, "cannot create resource tree client") } + return xrmClient, nil } @@ -89,7 +92,8 @@ func InitializeClients(ctx context.Context, logger logging.Logger, clients ...In clientType := reflect.TypeOf(c).String() logger.Debug("Initializing client", "type", clientType) - if err := c.Initialize(ctx); err != nil { + err := c.Initialize(ctx) + if err != nil { logger.Debug("Failed to initialize client", "type", clientType, "error", err) errs = append(errs, fmt.Errorf("failed to initialize %s: %w", clientType, err)) } @@ -98,5 +102,6 @@ func InitializeClients(ctx context.Context, logger logging.Logger, clients ...In if len(errs) > 0 { return errors.Join(errs...) } + return nil } diff --git a/cmd/diff/client/crossplane/base.go b/cmd/diff/client/crossplane/base.go index d8078ff..2186c10 100644 --- a/cmd/diff/client/crossplane/base.go +++ b/cmd/diff/client/crossplane/base.go @@ -52,6 +52,7 @@ func getFirstMatchingResource(ctx context.Context, client kubernetes.ResourceCli } var errs []error + for _, gvk := range gvks { // try for this version item, err := client.GetResource(ctx, gvk, namespace, name) @@ -73,11 +74,13 @@ func getFirstMatchingResource(ctx context.Context, client kubernetes.ResourceCli func listMatchingResources(ctx context.Context, client kubernetes.ResourceClient, gvks []schema.GroupVersionKind, namespace string) ([]*un.Unstructured, error) { // List all matching resources for the given GVKs var envConfigs []*un.Unstructured + for _, gvk := range gvks { ecs, err := client.ListResources(ctx, gvk, namespace) if err != nil { return nil, errors.Wrapf(err, "cannot list items for GVK %s", gvk.String()) } + envConfigs = append(envConfigs, ecs...) } diff --git a/cmd/diff/client/crossplane/composition_client.go b/cmd/diff/client/crossplane/composition_client.go index 4448ba2..c26f672 100644 --- a/cmd/diff/client/crossplane/composition_client.go +++ b/cmd/diff/client/crossplane/composition_client.go @@ -59,6 +59,7 @@ func (c *DefaultCompositionClient) Initialize(ctx context.Context) error { if err != nil { return errors.Wrap(err, "cannot get Composition GVKs") } + c.gvks = gvks // List compositions to populate the cache @@ -73,6 +74,7 @@ func (c *DefaultCompositionClient) Initialize(ctx context.Context) error { } c.logger.Debug("Composition client initialized", "compositionsCount", len(c.compositions)) + return nil } @@ -101,16 +103,21 @@ func (c *DefaultCompositionClient) ListCompositions(ctx context.Context) ([]*api compositions := make([]*apiextensionsv1.Composition, 0, len(unComps)) for _, obj := range unComps { comp := &apiextensionsv1.Composition{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, comp); err != nil { + + err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, comp) + if err != nil { c.logger.Debug("Failed to convert composition from unstructured", "name", obj.GetName(), "error", err) + return nil, errors.Wrap(err, "cannot convert unstructured to Composition") } + compositions = append(compositions, comp) } c.logger.Debug("Successfully retrieved compositions", "count", len(compositions)) + return compositions, nil } @@ -165,6 +172,7 @@ func (c *DefaultCompositionClient) FindMatchingComposition(ctx context.Context, // If it's a claim, we need to find compositions for the corresponding XR type var targetGVK schema.GroupVersionKind + switch { case xrd != nil: targetGVK, err = c.getXRTypeFromXRD(xrd, resourceID) @@ -176,6 +184,7 @@ func (c *DefaultCompositionClient) FindMatchingComposition(ctx context.Context, c.logger.Debug("Resource is not a claim type, looking for XRD for XR", "resource", resourceID, "targetGVK", targetGVK.String()) + xrd, err = c.definitionClient.GetXRDForXR(ctx, gvk) if err != nil { return nil, errors.Wrapf(err, "resource %s requires its XR type to find a composition", resourceID) @@ -210,6 +219,7 @@ func (c *DefaultCompositionClient) getXRTypeFromXRD(xrdForClaim *un.Unstructured // Find the referenceable version - there should be exactly one xrVersion := "" + versions, versionsFound, _ := un.NestedSlice(xrdForClaim.Object, "spec", "versions") if versionsFound && len(versions) > 0 { // Look for the one version that is marked referenceable @@ -259,11 +269,13 @@ func (c *DefaultCompositionClient) labelsMatch(labels, selector map[string]strin return false } } + return true } func makeCrossplaneRefPath(apiVersion string, path ...string) []string { var specCrossplane []string + switch apiVersion { case "apiextensions.crossplane.io/v1": // Crossplane v1 keeps these under spec.x @@ -272,6 +284,7 @@ func makeCrossplaneRefPath(apiVersion string, path ...string) []string { // Crossplane v2 keeps these under spec.crossplane.x specCrossplane = []string{"spec", "crossplane"} } + return append(specCrossplane, path...) } @@ -299,8 +312,10 @@ func (c *DefaultCompositionClient) findByDirectReference(ctx context.Context, xr c.logger.Debug("Found composition by direct reference", "resource", resourceID, "composition", comp.GetName()) + return comp, nil } + return nil, nil // No direct reference found } @@ -350,6 +365,7 @@ func (c *DefaultCompositionClient) findByLabelSelector(ctx context.Context, xrd, c.logger.Debug("Found composition by label selector", "resource", resourceID, "composition", matchingCompositions[0].GetName()) + return matchingCompositions[0], nil default: // Multiple matches - this is ambiguous and should fail @@ -357,6 +373,7 @@ func (c *DefaultCompositionClient) findByLabelSelector(ctx context.Context, xrd, for i, comp := range matchingCompositions { names[i] = comp.GetName() } + return nil, errors.New("ambiguous composition selection: multiple compositions match") } } @@ -385,6 +402,7 @@ func (c *DefaultCompositionClient) findByTypeReference(ctx context.Context, _ *u if len(compatibleCompositions) == 0 { c.logger.Debug("No matching composition found", "targetGVK", targetGVK.String()) + return nil, errors.Errorf("no composition found for %s", targetGVK.String()) } @@ -395,6 +413,7 @@ func (c *DefaultCompositionClient) findByTypeReference(ctx context.Context, _ *u for i, comp := range compatibleCompositions { names[i] = comp.GetName() } + return nil, errors.Errorf("ambiguous composition selection: multiple compositions exist for %s", targetGVK.String()) } @@ -402,5 +421,6 @@ func (c *DefaultCompositionClient) findByTypeReference(ctx context.Context, _ *u c.logger.Debug("Found matching composition by type reference", "resource_name", resourceID, "composition_name", compatibleCompositions[0].GetName()) + return compatibleCompositions[0], nil } diff --git a/cmd/diff/client/crossplane/composition_client_test.go b/cmd/diff/client/crossplane/composition_client_test.go index e6ae2d9..8ca121b 100644 --- a/cmd/diff/client/crossplane/composition_client_test.go +++ b/cmd/diff/client/crossplane/composition_client_test.go @@ -64,6 +64,7 @@ func TestDefaultCompositionClient_FindMatchingComposition(t *testing.T) { "environment": "production", "tier": "standard", }) + return comp }() @@ -74,6 +75,7 @@ func TestDefaultCompositionClient_FindMatchingComposition(t *testing.T) { comp.SetLabels(map[string]string{ "environment": "production", }) + return comp }() @@ -84,6 +86,7 @@ func TestDefaultCompositionClient_FindMatchingComposition(t *testing.T) { comp.SetLabels(map[string]string{ "environment": "production", }) + return comp }() @@ -757,6 +760,7 @@ func TestDefaultCompositionClient_FindMatchingComposition(t *testing.T) { t.Errorf("\n%s\nFindMatchingComposition(...): expected error containing %q, got %q", tt.reason, tt.want.err.Error(), err.Error()) } + return } @@ -851,6 +855,7 @@ func TestDefaultCompositionClient_GetComposition(t *testing.T) { if err == nil { t.Errorf("\n%s\nGetComposition(...): expected error but got none", tt.reason) } + return } @@ -970,6 +975,7 @@ func TestDefaultCompositionClient_ListCompositions(t *testing.T) { t.Errorf("\n%s\nListCompositions(...): expected error containing %q, got %q", tt.reason, tt.errorContains, err.Error()) } + return } @@ -981,12 +987,14 @@ func TestDefaultCompositionClient_ListCompositions(t *testing.T) { if len(comps) != len(tt.expectComps) { t.Errorf("\n%s\nListCompositions(...): expected %d compositions, got %d", tt.reason, len(tt.expectComps), len(comps)) + return } // Check that we got the expected compositions for i, expected := range tt.expectComps { found := false + for _, actual := range comps { if actual.GetName() == expected.GetName() { found = true diff --git a/cmd/diff/client/crossplane/definition_client.go b/cmd/diff/client/crossplane/definition_client.go index 529e108..2710563 100644 --- a/cmd/diff/client/crossplane/definition_client.go +++ b/cmd/diff/client/crossplane/definition_client.go @@ -62,6 +62,7 @@ func (c *DefaultDefinitionClient) Initialize(ctx context.Context) error { if err != nil { return errors.Wrap(err, "cannot get XRD GVKs") } + c.gvks = gvks // Load XRDs @@ -71,6 +72,7 @@ func (c *DefaultDefinitionClient) Initialize(ctx context.Context) error { } c.logger.Debug("Definition client initialized", "xrdsCount", len(c.xrds)) + return nil } @@ -78,12 +80,15 @@ func (c *DefaultDefinitionClient) Initialize(ctx context.Context) error { func (c *DefaultDefinitionClient) GetXRDs(ctx context.Context) ([]*un.Unstructured, error) { // Check if XRDs are already loaded c.xrdsMutex.RLock() + if c.xrdsLoaded { xrds := c.xrds c.xrdsMutex.RUnlock() c.logger.Debug("Using cached XRDs", "count", len(xrds)) + return xrds, nil } + c.xrdsMutex.RUnlock() // Need to load XRDs @@ -110,6 +115,7 @@ func (c *DefaultDefinitionClient) GetXRDs(ctx context.Context) ([]*un.Unstructur c.xrdsLoaded = true c.logger.Debug("Successfully retrieved and cached XRDs", "count", len(xrds)) + return xrds, nil } @@ -187,6 +193,7 @@ func (c *DefaultDefinitionClient) GetXRDForXR(ctx context.Context, gvk schema.Gr } versionMatches := false + for _, v := range versions { version, ok := v.(map[string]interface{}) if !ok { @@ -227,5 +234,6 @@ func (c *DefaultDefinitionClient) IsClaimResource(ctx context.Context, resource } c.logger.Debug("Resource is a claim type", "gvk", gvk.String()) + return true } diff --git a/cmd/diff/client/crossplane/definition_client_test.go b/cmd/diff/client/crossplane/definition_client_test.go index ce8798c..d285ce4 100644 --- a/cmd/diff/client/crossplane/definition_client_test.go +++ b/cmd/diff/client/crossplane/definition_client_test.go @@ -145,9 +145,11 @@ func TestDefaultDefinitionClient_GetXRDs(t *testing.T) { t.Errorf("\n%s\nGetXRDs(): expected error but got none", tt.reason) return } + if tt.errSubstring != "" && !strings.Contains(err.Error(), tt.errSubstring) { t.Errorf("\n%s\nGetXRDs(): expected error containing %q, got %q", tt.reason, tt.errSubstring, err.Error()) } + return } @@ -343,9 +345,11 @@ func TestDefaultDefinitionClient_GetXRDForClaim(t *testing.T) { t.Errorf("\n%s\nGetXRDForClaim(): expected error but got none", tt.reason) return } + if tt.errSubstring != "" && !strings.Contains(err.Error(), tt.errSubstring) { t.Errorf("\n%s\nGetXRDForClaim(): expected error containing %q, got %q", tt.reason, tt.errSubstring, err.Error()) } + return } @@ -550,9 +554,11 @@ func TestDefaultDefinitionClient_GetXRDForXR(t *testing.T) { t.Errorf("\n%s\nGetXRDForXR(): expected error but got none", tt.reason) return } + if tt.errSubstring != "" && !strings.Contains(err.Error(), tt.errSubstring) { t.Errorf("\n%s\nGetXRDForXR(): expected error containing %q, got %q", tt.reason, tt.errSubstring, err.Error()) } + return } diff --git a/cmd/diff/client/crossplane/environment_client.go b/cmd/diff/client/crossplane/environment_client.go index 903d754..abcdf76 100644 --- a/cmd/diff/client/crossplane/environment_client.go +++ b/cmd/diff/client/crossplane/environment_client.go @@ -50,6 +50,7 @@ func (c *DefaultEnvironmentClient) Initialize(ctx context.Context) error { if err != nil { return errors.Wrap(err, "cannot get EnvironmentConfig GVKs") } + c.gvks = gvks // List environment configs to populate the cache @@ -64,6 +65,7 @@ func (c *DefaultEnvironmentClient) Initialize(ctx context.Context) error { } c.logger.Debug("Environment client initialized", "envConfigsCount", len(c.envConfigs)) + return nil } @@ -77,6 +79,7 @@ func (c *DefaultEnvironmentClient) GetEnvironmentConfigs(ctx context.Context) ([ } c.logger.Debug("Environment configs retrieved", "count", len(envConfigs)) + return envConfigs, nil } diff --git a/cmd/diff/client/crossplane/environment_client_test.go b/cmd/diff/client/crossplane/environment_client_test.go index b0049cf..09b2449 100644 --- a/cmd/diff/client/crossplane/environment_client_test.go +++ b/cmd/diff/client/crossplane/environment_client_test.go @@ -111,9 +111,11 @@ func TestDefaultEnvironmentClient_GetEnvironmentConfigs(t *testing.T) { t.Errorf("\n%s\nGetEnvironmentConfigs(): expected error but got none", tt.reason) return } + if tt.errSubstring != "" && !strings.Contains(err.Error(), tt.errSubstring) { t.Errorf("\n%s\nGetEnvironmentConfigs(): expected error containing %q, got %q", tt.reason, tt.errSubstring, err.Error()) } + return } @@ -146,6 +148,7 @@ func TestDefaultEnvironmentClient_GetEnvironmentConfigs(t *testing.T) { } else { // Check data field for configs that exist in both lists wantData, _, _ := un.NestedMap(wantCfg.Object, "spec", "data") + gotData, _, _ := un.NestedMap(gotConfigs[name].Object, "spec", "data") if diff := cmp.Diff(wantData, gotData); diff != "" { t.Errorf("\n%s\nGetEnvironmentConfigs(): config %s data mismatch -want, +got:\n%s", tt.reason, name, diff) @@ -259,9 +262,11 @@ func TestDefaultEnvironmentClient_GetEnvironmentConfig(t *testing.T) { t.Errorf("\n%s\nGetEnvironmentConfig(): expected error but got none", tt.reason) return } + if tt.errSubstring != "" && !strings.Contains(err.Error(), tt.errSubstring) { t.Errorf("\n%s\nGetEnvironmentConfig(): expected error containing %q, got %q", tt.reason, tt.errSubstring, err.Error()) } + return } @@ -277,6 +282,7 @@ func TestDefaultEnvironmentClient_GetEnvironmentConfig(t *testing.T) { // Check that the data is correct wantData, _, _ := un.NestedMap(tt.want.Object, "spec", "data") + gotData, _, _ := un.NestedMap(got.Object, "spec", "data") if diff := cmp.Diff(wantData, gotData); diff != "" { t.Errorf("\n%s\nGetEnvironmentConfig(): config data mismatch -want, +got:\n%s", tt.reason, diff) diff --git a/cmd/diff/client/crossplane/function_client.go b/cmd/diff/client/crossplane/function_client.go index f4e0cb9..e10cd44 100644 --- a/cmd/diff/client/crossplane/function_client.go +++ b/cmd/diff/client/crossplane/function_client.go @@ -54,6 +54,7 @@ func (c *DefaultFunctionClient) Initialize(ctx context.Context) error { if err != nil { return errors.Wrap(err, "cannot get Function GVKs") } + c.gvks = gvks // List functions to populate the cache @@ -68,6 +69,7 @@ func (c *DefaultFunctionClient) Initialize(ctx context.Context) error { } c.logger.Debug("Function client initialized", "functionsCount", len(c.functions)) + return nil } @@ -96,16 +98,21 @@ func (c *DefaultFunctionClient) ListFunctions(ctx context.Context) ([]pkgv1.Func functions := make([]pkgv1.Function, 0, len(unFns)) for _, obj := range unFns { fn := pkgv1.Function{} - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, &fn); err != nil { + + err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, &fn) + if err != nil { c.logger.Debug("Failed to convert function from unstructured", "name", obj.GetName(), "error", err) + return nil, errors.Wrap(err, "cannot convert unstructured to Function") } + functions = append(functions, fn) } c.logger.Debug("Successfully retrieved functions", "count", len(functions)) + return functions, nil } @@ -120,6 +127,7 @@ func (c *DefaultFunctionClient) GetFunctionsFromPipeline(comp *apiextensionsv1.C // TODO: there was nil handling here that became invalid; make sure everything works as expected return string(comp.Spec.Mode) }()) + return nil, fmt.Errorf("unsupported composition Mode '%s'; supported types are [%s]", comp.Spec.Mode, apiextensionsv1.CompositionModePipeline) // TODO: we used to check for nil, and if nil we'd say "no mode found"; is it valid to have no mode? } @@ -133,8 +141,10 @@ func (c *DefaultFunctionClient) GetFunctionsFromPipeline(comp *apiextensionsv1.C c.logger.Debug("Function not found", "step", step.Step, "function_name", step.FunctionRef.Name) + return nil, errors.Errorf("function %q referenced in pipeline step %q not found", step.FunctionRef.Name, step.Step) } + c.logger.Debug("Found function for step", "step", step.Step, "function_name", fn.GetName()) @@ -144,5 +154,6 @@ func (c *DefaultFunctionClient) GetFunctionsFromPipeline(comp *apiextensionsv1.C c.logger.Debug("Retrieved functions from pipeline", "functions_count", len(functions), "composition_name", comp.GetName()) + return functions, nil } diff --git a/cmd/diff/client/crossplane/function_client_test.go b/cmd/diff/client/crossplane/function_client_test.go index d1b44c5..87a2ad6 100644 --- a/cmd/diff/client/crossplane/function_client_test.go +++ b/cmd/diff/client/crossplane/function_client_test.go @@ -270,6 +270,7 @@ func TestDefaultFunctionClient_GetFunctionsFromPipeline(t *testing.T) { if diff := cmp.Diff(tt.want.err.Error(), err.Error()); diff != "" { t.Errorf("\n%s\nGetFunctionsFromPipeline(...): -want error, +got error:\n%s", tt.reason, diff) } + return } @@ -282,6 +283,7 @@ func TestDefaultFunctionClient_GetFunctionsFromPipeline(t *testing.T) { if got != nil { t.Errorf("\n%s\nGetFunctionsFromPipeline(...): expected nil functions, got %v", tt.reason, got) } + return } @@ -294,6 +296,7 @@ func TestDefaultFunctionClient_GetFunctionsFromPipeline(t *testing.T) { if i >= len(got) { break } + if diff := cmp.Diff(wantFn.GetName(), got[i].GetName()); diff != "" { t.Errorf("\n%s\nGetFunctionsFromPipeline(...): -want function name, +got function name at index %d:\n%s", tt.reason, i, diff) } @@ -430,9 +433,11 @@ func TestDefaultFunctionClient_ListFunctions(t *testing.T) { t.Errorf("\n%s\nListFunctions(): expected error but got none", tt.reason) return } + if tt.errSubstring != "" && !strings.Contains(err.Error(), tt.errSubstring) { t.Errorf("\n%s\nListFunctions(): expected error containing %q, got %q", tt.reason, tt.errSubstring, err.Error()) } + return } @@ -587,6 +592,7 @@ func TestDefaultFunctionClient_Initialize(t *testing.T) { if err == nil { t.Errorf("\n%s\nInitialize(): expected error but got none", tt.reason) } + return } diff --git a/cmd/diff/client/crossplane/resource_tree_client.go b/cmd/diff/client/crossplane/resource_tree_client.go index 06c12e3..5ac776b 100644 --- a/cmd/diff/client/crossplane/resource_tree_client.go +++ b/cmd/diff/client/crossplane/resource_tree_client.go @@ -60,6 +60,7 @@ func (c *DefaultResourceTreeClient) GetResourceTree(ctx context.Context, root *u "resource_kind", root.GetKind(), "resource_name", root.GetName(), "error", err) + return nil, errors.Wrap(err, "failed to get resource tree") } diff --git a/cmd/diff/client/kubernetes/apply_client.go b/cmd/diff/client/kubernetes/apply_client.go index 48134ad..5093873 100644 --- a/cmd/diff/client/kubernetes/apply_client.go +++ b/cmd/diff/client/kubernetes/apply_client.go @@ -64,10 +64,12 @@ func (c *DefaultApplyClient) DryRunApply(ctx context.Context, obj *un.Unstructur result, err := resourceClient.Apply(ctx, obj.GetName(), obj, applyOptions) if err != nil { c.logger.Debug("Dry-run apply failed", "resource", resourceID, "error", err) + return nil, errors.Wrapf(err, "failed to apply resource %s/%s", obj.GetNamespace(), obj.GetName()) } c.logger.Debug("Dry-run apply successful", "resource", resourceID, "resourceVersion", result.GetResourceVersion()) + return result, nil } diff --git a/cmd/diff/client/kubernetes/apply_client_test.go b/cmd/diff/client/kubernetes/apply_client_test.go index 62a98d2..bf30693 100644 --- a/cmd/diff/client/kubernetes/apply_client_test.go +++ b/cmd/diff/client/kubernetes/apply_client_test.go @@ -49,7 +49,7 @@ func TestApplyClient_DryRunApply(t *testing.T) { // Create dynamic client that returns the object with a resource version dynamicClient := fake.NewSimpleDynamicClient(scheme) // Add reactor to handle apply operation - dynamicClient.Fake.PrependReactor("patch", "exampleresources", func(kt.Action) (bool, runtime.Object, error) { + dynamicClient.PrependReactor("patch", "exampleresources", func(kt.Action) (bool, runtime.Object, error) { // For apply, we'd return the "server-modified" version result := obj.DeepCopy() result.SetResourceVersion("1000") // Server would set this @@ -92,7 +92,7 @@ func TestApplyClient_DryRunApply(t *testing.T) { // Create dynamic client that returns the object with a resource version dynamicClient := fake.NewSimpleDynamicClient(scheme) // Add reactor to handle apply operation - dynamicClient.Fake.PrependReactor("patch", "clusterresources", func(kt.Action) (bool, runtime.Object, error) { + dynamicClient.PrependReactor("patch", "clusterresources", func(kt.Action) (bool, runtime.Object, error) { // For apply, we'd return the "server-modified" version result := obj.DeepCopy() result.SetResourceVersion("1000") // Server would set this @@ -152,7 +152,7 @@ func TestApplyClient_DryRunApply(t *testing.T) { setup: func() (dynamic.Interface, TypeConverter) { dynamicClient := fake.NewSimpleDynamicClient(scheme) // Add reactor to make apply fail - dynamicClient.Fake.PrependReactor("patch", "exampleresources", func(kt.Action) (bool, runtime.Object, error) { + dynamicClient.PrependReactor("patch", "exampleresources", func(kt.Action) (bool, runtime.Object, error) { return true, nil, errors.New("apply failed") }) @@ -203,6 +203,7 @@ func TestApplyClient_DryRunApply(t *testing.T) { t.Errorf("\n%s\nDryRunApply(...): expected error containing %q, got %q", tc.reason, tc.want.err.Error(), err.Error()) } + return } diff --git a/cmd/diff/client/kubernetes/resource_client.go b/cmd/diff/client/kubernetes/resource_client.go index 55234cc..65a627b 100644 --- a/cmd/diff/client/kubernetes/resource_client.go +++ b/cmd/diff/client/kubernetes/resource_client.go @@ -74,6 +74,7 @@ func (c *DefaultResourceClient) GetResource(ctx context.Context, gvk schema.Grou "resource", resourceID, "uid", res.GetUID(), "resourceVersion", res.GetResourceVersion()) + return res, nil } @@ -111,6 +112,7 @@ func (c *DefaultResourceClient) GetResourcesByLabel(ctx context.Context, gvk sch } c.logger.Debug("Resources found by label", "count", len(resources), "gvk", gvk.String()) + return resources, nil } @@ -139,6 +141,7 @@ func (c *DefaultResourceClient) ListResources(ctx context.Context, gvk schema.Gr } c.logger.Debug("Listed resources", "gvk", gvk.String(), "namespace", namespace, "count", len(resources)) + return resources, nil } @@ -174,6 +177,7 @@ func (c *DefaultResourceClient) GetGVKsForGroupKind(_ context.Context, group, ki Kind: kind, } gvks = append(gvks, gvk) + break // Found the kind in this version, move to next version } } @@ -187,6 +191,7 @@ func (c *DefaultResourceClient) GetGVKsForGroupKind(_ context.Context, group, ki func (c *DefaultResourceClient) IsNamespacedResource(_ context.Context, gvk schema.GroupVersionKind) (bool, error) { // Get the server resources for this group/version groupVersion := gvk.GroupVersion().String() + resourceList, err := c.discoveryClient.ServerResourcesForGroupVersion(groupVersion) if err != nil { return false, errors.Wrapf(err, "cannot get server resources for group version %s", groupVersion) @@ -199,6 +204,7 @@ func (c *DefaultResourceClient) IsNamespacedResource(_ context.Context, gvk sche c.logger.Debug("Determined resource scope from discovery", "gvk", gvk.String(), "namespaced", resource.Namespaced) + return resource.Namespaced, nil } } @@ -208,5 +214,6 @@ func (c *DefaultResourceClient) IsNamespacedResource(_ context.Context, gvk sche for i, resource := range resourceList.APIResources { availableKinds[i] = resource.Kind } + return false, errors.Errorf("resource kind %s not found in discovery API for group version %s (available kinds: %v)", gvk.Kind, groupVersion, availableKinds) } diff --git a/cmd/diff/client/kubernetes/resource_client_test.go b/cmd/diff/client/kubernetes/resource_client_test.go index 7c2f923..426d886 100644 --- a/cmd/diff/client/kubernetes/resource_client_test.go +++ b/cmd/diff/client/kubernetes/resource_client_test.go @@ -126,7 +126,7 @@ func TestResourceClient_GetResource(t *testing.T) { reason: "Should return an error when the resource doesn't exist", setup: func() (dynamic.Interface, TypeConverter) { dc := fake.NewSimpleDynamicClient(scheme) - dc.Fake.PrependReactor("get", "*", func(kt.Action) (bool, runtime.Object, error) { + dc.PrependReactor("get", "*", func(kt.Action) (bool, runtime.Object, error) { return true, nil, errors.New("resource not found") }) @@ -209,6 +209,7 @@ func TestResourceClient_GetResource(t *testing.T) { t.Errorf("\n%s\nGetResource(...): expected error containing %q, got %q", tc.reason, tc.want.err.Error(), err.Error()) } + return } @@ -389,7 +390,7 @@ func TestResourceClient_GetResourcesByLabel(t *testing.T) { {Group: "example.org", Version: "v1", Resource: "resources"}: "ResourceList", }) - dc.Fake.PrependReactor("list", "resources", func(kt.Action) (bool, runtime.Object, error) { + dc.PrependReactor("list", "resources", func(kt.Action) (bool, runtime.Object, error) { return true, nil, errors.New("list error") }) @@ -491,6 +492,7 @@ func TestResourceClient_GetResourcesByLabel(t *testing.T) { t.Errorf("\n%s\nGetResourcesByLabel(...): expected error containing %q, got: %v", tc.reason, tc.want.err.Error(), err) } + return } @@ -641,7 +643,7 @@ func TestResourceClient_ListResources(t *testing.T) { {Group: "example.org", Version: "v1", Resource: "resources"}: "ResourceList", }) - dc.Fake.PrependReactor("list", "resources", func(kt.Action) (bool, runtime.Object, error) { + dc.PrependReactor("list", "resources", func(kt.Action) (bool, runtime.Object, error) { return true, nil, errors.New("list error") }) @@ -720,6 +722,7 @@ func TestResourceClient_ListResources(t *testing.T) { t.Errorf("\n%s\nListResources(...): expected error containing %q, got %q", tc.reason, tc.want.err.Error(), err.Error()) } + return } @@ -916,10 +919,12 @@ func TestResourceClient_IsNamespacedResource(t *testing.T) { t.Errorf("\n%s\nIsNamespacedResource(...): expected error but got none", tc.reason) return } + if tc.errMsg != "" && !strings.Contains(err.Error(), tc.errMsg) { t.Errorf("\n%s\nIsNamespacedResource(...): expected error containing %q, got %q", tc.reason, tc.errMsg, err.Error()) } + return } diff --git a/cmd/diff/client/kubernetes/schema_client.go b/cmd/diff/client/kubernetes/schema_client.go index 2295a4c..a576e84 100644 --- a/cmd/diff/client/kubernetes/schema_client.go +++ b/cmd/diff/client/kubernetes/schema_client.go @@ -77,6 +77,7 @@ func (c *DefaultSchemaClient) GetCRD(ctx context.Context, gvk schema.GroupVersio } c.logger.Debug("Successfully retrieved CRD", "gvk", gvk.String(), "crdName", resourceName) + return crd, nil } @@ -84,10 +85,12 @@ func (c *DefaultSchemaClient) GetCRD(ctx context.Context, gvk schema.GroupVersio func (c *DefaultSchemaClient) IsCRDRequired(ctx context.Context, gvk schema.GroupVersionKind) bool { // Check cache first c.resourceMapMu.RLock() + if val, ok := c.resourceTypeMap[gvk]; ok { c.resourceMapMu.RUnlock() return val } + c.resourceMapMu.RUnlock() // Core API resources never need CRDs @@ -122,11 +125,13 @@ func (c *DefaultSchemaClient) IsCRDRequired(ctx context.Context, gvk schema.Grou "gvk", gvk.String(), "error", err) c.cacheResourceType(gvk, true) + return true } // Default to requiring a CRD c.cacheResourceType(gvk, true) + return true } @@ -141,5 +146,6 @@ func (c *DefaultSchemaClient) ValidateResource(_ context.Context, resource *un.U func (c *DefaultSchemaClient) cacheResourceType(gvk schema.GroupVersionKind, requiresCRD bool) { c.resourceMapMu.Lock() defer c.resourceMapMu.Unlock() + c.resourceTypeMap[gvk] = requiresCRD } diff --git a/cmd/diff/client/kubernetes/schema_client_test.go b/cmd/diff/client/kubernetes/schema_client_test.go index a268b7b..f762c8e 100644 --- a/cmd/diff/client/kubernetes/schema_client_test.go +++ b/cmd/diff/client/kubernetes/schema_client_test.go @@ -201,7 +201,7 @@ func TestSchemaClient_GetCRD(t *testing.T) { setup: func() (dynamic.Interface, TypeConverter) { // Set up the dynamic client to return our test CRD dynamicClient := fake.NewSimpleDynamicClient(scheme) - dynamicClient.Fake.PrependReactor("get", "customresourcedefinitions", func(action kt.Action) (bool, runtime.Object, error) { + dynamicClient.PrependReactor("get", "customresourcedefinitions", func(action kt.Action) (bool, runtime.Object, error) { getAction := action.(kt.GetAction) if getAction.GetName() == "xresources.example.org" { return true, testCRD, nil @@ -237,7 +237,7 @@ func TestSchemaClient_GetCRD(t *testing.T) { reason: "Should return error when CRD doesn't exist", setup: func() (dynamic.Interface, TypeConverter) { dynamicClient := fake.NewSimpleDynamicClient(scheme) - dynamicClient.Fake.PrependReactor("get", "customresourcedefinitions", func(kt.Action) (bool, runtime.Object, error) { + dynamicClient.PrependReactor("get", "customresourcedefinitions", func(kt.Action) (bool, runtime.Object, error) { return true, nil, errors.New("CRD not found") }) @@ -316,6 +316,7 @@ func TestSchemaClient_GetCRD(t *testing.T) { t.Errorf("\n%s\nGetCRD(...): expected error containing %q, got %q", tc.reason, tc.want.err.Error(), err.Error()) } + return } diff --git a/cmd/diff/client/kubernetes/type_converter.go b/cmd/diff/client/kubernetes/type_converter.go index c502c75..e8675f0 100644 --- a/cmd/diff/client/kubernetes/type_converter.go +++ b/cmd/diff/client/kubernetes/type_converter.go @@ -47,10 +47,12 @@ func NewTypeConverter(clients *core.Clients, logger logging.Logger) TypeConverte func (c *DefaultTypeConverter) GVKToGVR(ctx context.Context, gvk schema.GroupVersionKind) (schema.GroupVersionResource, error) { // Use the cached mapping if we have it c.gvkToGVRMutex.RLock() + if gvr, ok := c.gvkToGVRMap[gvk]; ok { c.gvkToGVRMutex.RUnlock() return gvr, nil } + c.gvkToGVRMutex.RUnlock() // Get the resource name diff --git a/cmd/diff/client/kubernetes/type_converter_test.go b/cmd/diff/client/kubernetes/type_converter_test.go index 3c1d077..62a9176 100644 --- a/cmd/diff/client/kubernetes/type_converter_test.go +++ b/cmd/diff/client/kubernetes/type_converter_test.go @@ -162,6 +162,7 @@ func TestTypeConverter_GVKToGVR(t *testing.T) { t.Errorf("\n%s\nGVKToGVR(...): expected error containing %q, got %q", tc.reason, tc.want.err.Error(), err.Error()) } + return } @@ -347,6 +348,7 @@ func TestTypeConverter_GetResourceNameForGVK(t *testing.T) { t.Errorf("\n%s\nGetResourceNameForGVK(...): expected error containing %q, got %q", tc.reason, tc.want.err.Error(), err.Error()) } + return } diff --git a/cmd/diff/diff.go b/cmd/diff/diff.go index 301a003..1eb3225 100644 --- a/cmd/diff/diff.go +++ b/cmd/diff/diff.go @@ -78,10 +78,12 @@ func (c *Cmd) AfterApply(ctx *kong.Context, log logging.Logger, config *rest.Con func (c *Cmd) initializeDependencies(ctx *kong.Context, log logging.Logger, config *rest.Config) error { config = c.initRestConfig(config, log) + appCtx, err := NewAppContext(config, log) if err != nil { return errors.Wrap(err, "cannot create app context") } + proc := makeDefaultProc(c, appCtx, log) loader, err := makeDefaultLoader(c) @@ -92,6 +94,7 @@ func (c *Cmd) initializeDependencies(ctx *kong.Context, log logging.Logger, conf ctx.Bind(appCtx) ctx.BindTo(proc, (*dp.DiffProcessor)(nil)) ctx.BindTo(loader, (*ld.Loader)(nil)) + return nil } @@ -133,6 +136,7 @@ func makeDefaultProc(c *Cmd, ctx *AppContext, log logging.Logger) dp.DiffProcess dp.WithColorize(!c.NoColor), dp.WithCompact(c.Compact), } + return dp.NewDiffProcessor(ctx.K8sClients, ctx.XpClients, options...) } @@ -152,7 +156,6 @@ func (c *Cmd) Run(k *kong.Context, log logging.Logger, appCtx *AppContext, proc // TODO: diff against upgraded schema that isn't applied yet // TODO: diff against upgraded composition that isn't applied yet // TODO: diff against upgraded composition version that is already available - ctx, cancel := context.WithTimeout(context.Background(), c.Timeout) defer cancel() diff --git a/cmd/diff/diff_integration_test.go b/cmd/diff/diff_integration_test.go index fff44d2..a9168ab 100644 --- a/cmd/diff/diff_integration_test.go +++ b/cmd/diff/diff_integration_test.go @@ -1084,7 +1084,8 @@ Summary: 2 modified`, // Ensure we clean up at the end of the test defer func() { - if err := testEnv.Stop(); err != nil { + err := testEnv.Stop() + if err != nil { t.Logf("failed to stop test environment: %v", err) } }() @@ -1111,26 +1112,31 @@ Summary: 2 modified`, // Apply resources with owner references if len(tt.setupFilesWithOwnerRefs) > 0 { - if err := applyHierarchicalOwnership(ctx, tu.TestLogger(t, false), k8sClient, xrdAPIVersion, tt.setupFilesWithOwnerRefs); err != nil { + err := applyHierarchicalOwnership(ctx, tu.TestLogger(t, false), k8sClient, xrdAPIVersion, tt.setupFilesWithOwnerRefs) + if err != nil { t.Fatalf("failed to setup owner references: %v", err) } } // Set up the test file tempDir := t.TempDir() + var testFiles []string // Handle any additional input files for i, inputFile := range tt.inputFiles { testFile := filepath.Join(tempDir, fmt.Sprintf("test_%d.yaml", i)) + content, err := os.ReadFile(inputFile) if err != nil { t.Fatalf("failed to read input file: %v", err) } + err = os.WriteFile(testFile, content, 0o644) if err != nil { t.Fatalf("failed to write test file: %v", err) } + testFiles = append(testFiles, testFile) } @@ -1175,6 +1181,7 @@ Summary: 2 modified`, if tt.expectedError && err == nil { t.Fatal("expected error but got none") } + if !tt.expectedError && err != nil { t.Fatalf("expected no error but got: %v", err) } @@ -1202,11 +1209,13 @@ Summary: 2 modified`, // Strings aren't equal, *including* ansi. but we can compare ignoring ansi to determine what output to // show for the failure. if the difference is only in color codes, we'll show escaped ansi codes. out := outputStr + expect := tt.expectedOutput if tu.CompareIgnoringAnsi(strings.TrimSpace(outputStr), strings.TrimSpace(tt.expectedOutput)) { out = strconv.QuoteToASCII(outputStr) expect = strconv.QuoteToASCII(tt.expectedOutput) } + t.Fatalf("expected output to contain:\n%s\n\nbut got:\n%s", expect, out) } }) diff --git a/cmd/diff/diff_it_utils_test.go b/cmd/diff/diff_it_utils_test.go index a7ee3a2..675183a 100644 --- a/cmd/diff/diff_it_utils_test.go +++ b/cmd/diff/diff_it_utils_test.go @@ -290,6 +290,7 @@ func addResourceRef(parent, child *un.Unstructured, xrAPIVersion XrdAPIVersion) } var resourceRefsPath []string + switch xrAPIVersion { case V1: resourceRefsPath = []string{"spec", "resourceRefs"} @@ -309,6 +310,7 @@ func addResourceRef(parent, child *un.Unstructured, xrAPIVersion XrdAPIVersion) // Add the new reference and update the parent resourceRefs = append(resourceRefs, ref) + return un.SetNestedSlice(parent.Object, resourceRefs, resourceRefsPath...) } @@ -317,11 +319,13 @@ func addResourceRef(parent, child *un.Unstructured, xrAPIVersion XrdAPIVersion) func applyResourcesFromFiles(ctx context.Context, c client.Client, paths []string) error { // Collect all resources from all files first var allResources []*un.Unstructured + for _, path := range paths { resources, err := readResourcesFromFile(path) if err != nil { return fmt.Errorf("failed to read resources from %s: %w", path, err) } + allResources = append(allResources, resources...) } @@ -338,15 +342,18 @@ func readResourcesFromFile(path string) ([]*un.Unstructured, error) { // Use a YAML decoder to handle multiple documents decoder := yaml.NewYAMLOrJSONDecoder(bytes.NewReader(data), 4096) + var resources []*un.Unstructured for { resource := &un.Unstructured{} + err := decoder.Decode(resource) if err != nil { if errors.Is(err, io.EOF) { break } + return nil, fmt.Errorf("failed to decode YAML document from %s: %w", path, err) } @@ -365,15 +372,18 @@ func readResourcesFromFile(path string) ([]*un.Unstructured, error) { // Assumes resources don't already exist - fails if they do. func createResources(ctx context.Context, c client.Client, resources []*un.Unstructured) error { for _, resource := range resources { - if err := c.Create(ctx, resource.DeepCopy()); err != nil { + err := c.Create(ctx, resource.DeepCopy()) + if err != nil { if apierrors.IsAlreadyExists(err) { return fmt.Errorf("resource %s/%s of kind %s already exists - test setup error", resource.GetNamespace(), resource.GetName(), resource.GetKind()) } + return fmt.Errorf("failed to create resource %s/%s: %w", resource.GetNamespace(), resource.GetName(), err) } } + return nil } @@ -385,12 +395,14 @@ func applyHierarchicalOwnership(ctx context.Context, _ logging.Logger, c client. parentChildRelationships := make(map[string]string) // child file -> parent file // First pass: Create all resources and collect parent-child relationships - if err := createAllResourcesInHierarchy(ctx, c, hierarchies, createdResources, parentChildRelationships); err != nil { + err := createAllResourcesInHierarchy(ctx, c, hierarchies, createdResources, parentChildRelationships) + if err != nil { return err } // Second pass: Apply all owner references and resource refs between parents and children - if err := applyAllRelationships(ctx, c, createdResources, parentChildRelationships, xrdAPIVersion); err != nil { + err = applyAllRelationships(ctx, c, createdResources, parentChildRelationships, xrdAPIVersion) + if err != nil { return err } @@ -413,6 +425,7 @@ func LogResourcesAsYAML(ctx context.Context, log logging.Logger, c client.Client for filePath := range createdResources { filePaths = append(filePaths, filePath) } + sort.Strings(filePaths) for _, filePath := range filePaths { @@ -442,6 +455,7 @@ func LogResourcesAsYAML(ctx context.Context, log logging.Logger, c client.Client } log.Info("===== END OF RESOURCES =====\n\n") + return nil } @@ -478,8 +492,9 @@ func createAllResourcesInHierarchy(ctx context.Context, c client.Client, }, } - if err := createAllResourcesInHierarchy(ctx, c, childHierarchies, - createdResources, parentChildRelationships); err != nil { + err := createAllResourcesInHierarchy(ctx, c, childHierarchies, + createdResources, parentChildRelationships) + if err != nil { return err } } @@ -517,17 +532,20 @@ func createResourceFromFile(ctx context.Context, c client.Client, path string, existing := &un.Unstructured{} existing.SetGroupVersionKind(resource.GroupVersionKind()) - if err := c.Get(ctx, client.ObjectKey{ + err := c.Get(ctx, client.ObjectKey{ Name: resource.GetName(), Namespace: resource.GetNamespace(), - }, existing); err != nil { + }, existing) + if err != nil { return nil, fmt.Errorf("failed to get existing resource: %w", err) } // Store and return the existing resource createdResources[path] = existing + return existing, nil } + return nil, fmt.Errorf("failed to create resource: %w", err) } @@ -544,6 +562,7 @@ func createResourceFromFile(ctx context.Context, c client.Client, path string, // Store and return the created resource createdResources[path] = serverResource + return serverResource, nil } @@ -564,12 +583,14 @@ func applyAllRelationships(ctx context.Context, c client.Client, } // 1. Set the owner reference in the child's metadata - if err := setOwnerReferenceAndUpdate(ctx, c, parentResource, childResource); err != nil { + err := setOwnerReferenceAndUpdate(ctx, c, parentResource, childResource) + if err != nil { return err } // 2. Add the child resource reference to the parent - if err := addResourceRefAndUpdate(ctx, c, parentResource, childResource, xrdAPIVersion); err != nil { + err = addResourceRefAndUpdate(ctx, c, parentResource, childResource, xrdAPIVersion) + if err != nil { return err } } @@ -585,10 +606,11 @@ func setOwnerReferenceAndUpdate(ctx context.Context, c client.Client, latestChild := &un.Unstructured{} latestChild.SetGroupVersionKind(child.GroupVersionKind()) - if err := c.Get(ctx, client.ObjectKey{ + err := c.Get(ctx, client.ObjectKey{ Name: child.GetName(), Namespace: child.GetNamespace(), - }, latestChild); err != nil { + }, latestChild) + if err != nil { return fmt.Errorf("failed to get child resource: %w", err) } @@ -596,7 +618,8 @@ func setOwnerReferenceAndUpdate(ctx context.Context, c client.Client, setOwnerReference(latestChild, owner) // Update the child - if err := c.Update(ctx, latestChild); err != nil { + err = c.Update(ctx, latestChild) + if err != nil { return fmt.Errorf("failed to update child with owner reference: %w", err) } @@ -611,20 +634,23 @@ func addResourceRefAndUpdate(ctx context.Context, c client.Client, latestOwner := &un.Unstructured{} latestOwner.SetGroupVersionKind(owner.GroupVersionKind()) - if err := c.Get(ctx, client.ObjectKey{ + err := c.Get(ctx, client.ObjectKey{ Name: owner.GetName(), Namespace: owner.GetNamespace(), - }, latestOwner); err != nil { + }, latestOwner) + if err != nil { return fmt.Errorf("failed to get owner for updating references: %w", err) } // Add the resource reference - if err := addResourceRef(latestOwner, owned, xrdAPIVersion); err != nil { + err = addResourceRef(latestOwner, owned, xrdAPIVersion) + if err != nil { return fmt.Errorf("unable to add resource ref: %w", err) } // Update the owner with the new reference - if err := c.Update(ctx, latestOwner); err != nil { + err = c.Update(ctx, latestOwner) + if err != nil { return fmt.Errorf("failed to update owner with resource reference: %w", err) } diff --git a/cmd/diff/diff_test.go b/cmd/diff/diff_test.go index b781906..1ed4607 100644 --- a/cmd/diff/diff_test.go +++ b/cmd/diff/diff_test.go @@ -55,10 +55,12 @@ func TestCmd_Run(t *testing.T) { if err != nil { t.Fatalf("Failed to create Kong parser: %v", err) } + kongCtx, err := parser.Parse([]string{}) if err != nil { t.Fatalf("Failed to parse Kong context: %v", err) } + kongCtx.Stdout = &buf // Create a buffer to capture output @@ -129,7 +131,8 @@ kind: TestResource metadata: name: test-resource ` - if err := os.WriteFile(tempFile, []byte(content), 0o600); err != nil { + err := os.WriteFile(tempFile, []byte(content), 0o600) + if err != nil { t.Fatalf("Failed to write temp file: %v", err) } @@ -233,7 +236,8 @@ kind: TestResource metadata: name: test-resource ` - if err := os.WriteFile(tempFile, []byte(content), 0o600); err != nil { + err := os.WriteFile(tempFile, []byte(content), 0o600) + if err != nil { t.Fatalf("Failed to write temp file: %v", err) } @@ -268,6 +272,7 @@ metadata: t.Errorf("Run() error = %v, wantErr %v", err, tc.wantErr) return } + if err != nil && tc.wantErrContains != "" { if !strings.Contains(err.Error(), tc.wantErrContains) { t.Errorf("Run() error = %v, wantErrContains %v", err, tc.wantErrContains) @@ -450,7 +455,8 @@ spec: func() *un.Unstructured { // Parse the YAML into an unstructured object obj := &un.Unstructured{} - if err := yaml.Unmarshal(xrYAML, &obj.Object); err != nil { + err := yaml.Unmarshal(xrYAML, &obj.Object) + if err != nil { t.Fatalf("Failed to unmarshal test XR: %v", err) } return obj @@ -526,7 +532,8 @@ spec: func() *un.Unstructured { // Parse the YAML into an unstructured object obj := &un.Unstructured{} - if err := yaml.Unmarshal(xrYAML, &obj.Object); err != nil { + err := yaml.Unmarshal(xrYAML, &obj.Object) + if err != nil { t.Fatalf("Failed to unmarshal test XR: %v", err) } return obj @@ -630,7 +637,8 @@ spec: func() *un.Unstructured { // Parse the YAML into an unstructured object obj := &un.Unstructured{} - if err := yaml.Unmarshal(xrYAML, &obj.Object); err != nil { + err := yaml.Unmarshal(xrYAML, &obj.Object) + if err != nil { t.Fatalf("Failed to unmarshal test XR: %v", err) } return obj @@ -736,7 +744,8 @@ spec: Resources: []*un.Unstructured{ func() *un.Unstructured { obj := &un.Unstructured{} - if err := yaml.Unmarshal(xrYAML, &obj.Object); err != nil { + err := yaml.Unmarshal(xrYAML, &obj.Object) + if err != nil { t.Fatalf("Failed to unmarshal test XR: %v", err) } return obj @@ -881,10 +890,12 @@ spec: if err != nil { t.Fatalf("Failed to create Kong parser: %v", err) } + kongCtx, err := parser.Parse([]string{}) if err != nil { t.Fatalf("Failed to parse Kong context: %v", err) } + kongCtx.Stdout = &buf // Create a logger @@ -916,9 +927,11 @@ spec: t.Errorf("Expected error but got none") return } + if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { t.Errorf("Expected error containing %q, got: %v", tt.errorContains, err) } + return } @@ -985,7 +998,8 @@ users: user: token: test-token ` - if err := os.WriteFile(tempFile, []byte(content), 0o600); err != nil { + err := os.WriteFile(tempFile, []byte(content), 0o600) + if err != nil { t.Fatalf("Failed to write temp kubeconfig: %v", err) } return tempFile @@ -1003,6 +1017,7 @@ users: t.Run(name, func(t *testing.T) { // Set the KUBECONFIG environment variable for this test originalKubeconfig := os.Getenv("KUBECONFIG") + defer func() { if originalKubeconfig != "" { t.Setenv("KUBECONFIG", originalKubeconfig) @@ -1031,10 +1046,12 @@ users: t.Errorf("Expected error but got none") return } + if !tc.expectError && err != nil { t.Errorf("Expected no error, got: %v", err) return } + if tc.errorContains != "" && !strings.Contains(err.Error(), tc.errorContains) { t.Errorf("Expected error containing %q, got: %v", tc.errorContains, err) return diff --git a/cmd/diff/diffprocessor/diff_calculator.go b/cmd/diff/diffprocessor/diff_calculator.go index 9de982e..50942f4 100644 --- a/cmd/diff/diffprocessor/diff_calculator.go +++ b/cmd/diff/diffprocessor/diff_calculator.go @@ -63,6 +63,7 @@ func (c *DefaultDiffCalculator) CalculateDiff(ctx context.Context, composite *un // Create a resource ID for logging purposes var resourceID string + switch { case name != "": resourceID = fmt.Sprintf("%s/%s", desired.GetKind(), name) @@ -118,6 +119,7 @@ func (c *DefaultDiffCalculator) CalculateDiff(ctx context.Context, composite *un "resource", resourceID, "name", desired.GetName(), "desired", desired) + wouldBeResult, err = c.applyClient.DryRunApply(ctx, desired) if err != nil { c.logger.Debug("Dry-run apply failed", "resource", resourceID, "error", err) @@ -128,7 +130,7 @@ func (c *DefaultDiffCalculator) CalculateDiff(ctx context.Context, composite *un } // Generate diff with the configured options - diff, err := renderer.GenerateDiffWithOptions(current, wouldBeResult, c.logger, c.diffOptions) + diff, err := renderer.GenerateDiffWithOptions(ctx, current, wouldBeResult, c.logger, c.diffOptions) if err != nil { c.logger.Debug("Failed to generate diff", "resource", resourceID, "error", err) return nil, err @@ -153,6 +155,7 @@ func (c *DefaultDiffCalculator) CalculateDiffs(ctx context.Context, xr *cmp.Unst "composedCount", len(desired.ComposedResources)) diffs := make(map[string]*dt.ResourceDiff) + var errs []error // Create a map to track resources that were rendered @@ -189,6 +192,7 @@ func (c *DefaultDiffCalculator) CalculateDiffs(ctx context.Context, xr *cmp.Unst c.logger.Debug("Skipping resource with empty name and generateName", "kind", kind, "apiVersion", apiVersion) + continue } @@ -196,6 +200,7 @@ func (c *DefaultDiffCalculator) CalculateDiffs(ctx context.Context, xr *cmp.Unst if err != nil { c.logger.Debug("Error calculating diff for composed resource", "resource", resourceID, "error", err) errs = append(errs, errors.Wrapf(err, "cannot calculate diff for %s", resourceID)) + continue } @@ -210,6 +215,7 @@ func (c *DefaultDiffCalculator) CalculateDiffs(ctx context.Context, xr *cmp.Unst if xrDiff.Current != nil { // it only makes sense to calculate removal of things if we have a current XR. c.logger.Debug("Finding resources to be removed", "xr", xrName) + removedDiffs, err := c.CalculateRemovedResourceDiffs(ctx, xrDiff.Current, renderedResources) if err != nil { c.logger.Debug("Error calculating removed resources (continuing)", "error", err) @@ -253,6 +259,7 @@ func (c *DefaultDiffCalculator) CalculateRemovedResourceDiffs(ctx context.Contex // Create a handler function to recursively traverse the tree and find composed resources var findRemovedResources func(node *resource.Resource) + findRemovedResources = func(node *resource.Resource) { // Skip the root (XR) node if _, hasAnno := node.Unstructured.GetAnnotations()["crossplane.io/composition-resource-name"]; hasAnno { @@ -268,11 +275,12 @@ func (c *DefaultDiffCalculator) CalculateRemovedResourceDiffs(ctx context.Contex // This resource exists but wasn't rendered - it will be removed c.logger.Debug("Resource will be removed", "resource", resourceID) - diff, err := renderer.GenerateDiffWithOptions(&node.Unstructured, nil, c.logger, c.diffOptions) + diff, err := renderer.GenerateDiffWithOptions(ctx, &node.Unstructured, nil, c.logger, c.diffOptions) if err != nil { c.logger.Debug("Cannot calculate removal diff (continuing)", "resource", resourceID, "error", err) + return } @@ -295,5 +303,6 @@ func (c *DefaultDiffCalculator) CalculateRemovedResourceDiffs(ctx context.Contex } c.logger.Debug("Found resources to be removed", "count", len(removedDiffs)) + return removedDiffs, nil } diff --git a/cmd/diff/diffprocessor/diff_calculator_test.go b/cmd/diff/diffprocessor/diff_calculator_test.go index 0bd6074..396e84e 100644 --- a/cmd/diff/diffprocessor/diff_calculator_test.go +++ b/cmd/diff/diffprocessor/diff_calculator_test.go @@ -42,6 +42,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { Build() const ParentXRName = "parent-xr" + composedResource := tu.NewResource("example.org/v1", "ComposedResource", "cpd-resource"). WithSpecField("field", "old-value"). WithLabels(map[string]string{ @@ -356,8 +357,10 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { if err == nil { t.Errorf("CalculateDiff() expected error but got none") } + return } + if err != nil { t.Fatalf("CalculateDiff() unexpected error: %v", err) } @@ -367,6 +370,7 @@ func TestDefaultDiffCalculator_CalculateDiff(t *testing.T) { if diff != nil { t.Errorf("CalculateDiff() expected nil diff but got: %v", diff) } + return } @@ -676,8 +680,10 @@ func TestDefaultDiffCalculator_CalculateDiffs(t *testing.T) { if err == nil { t.Errorf("CalculateDiffs() expected error but got none") } + return } + if err != nil { t.Fatalf("CalculateDiffs() unexpected error: %v", err) } @@ -850,6 +856,7 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { if err == nil { t.Errorf("CalculateRemovedResourceDiffs() expected error but got none") } + return } @@ -868,18 +875,21 @@ func TestDefaultDiffCalculator_CalculateRemovedResourceDiffs(t *testing.T) { for key := range diffs { t.Logf("Found resource to remove: %s", key) } + return } // Verify each expected removed resource is in the result for _, name := range tt.expectedRemoved { found := false + for key, diff := range diffs { if strings.Contains(key, name) && diff.DiffType == dt.DiffTypeRemoved { found = true break } } + if !found { t.Errorf("Expected to find %s marked for removal but did not", name) } diff --git a/cmd/diff/diffprocessor/diff_processor.go b/cmd/diff/diffprocessor/diff_processor.go index bc904c2..f905963 100644 --- a/cmd/diff/diffprocessor/diff_processor.go +++ b/cmd/diff/diffprocessor/diff_processor.go @@ -97,16 +97,19 @@ func (p *DefaultDiffProcessor) Initialize(ctx context.Context) error { p.config.Logger.Debug("Initializing diff processor") // Load CRDs (handled by the schema validator) - if err := p.initializeSchemaValidator(ctx); err != nil { + err := p.initializeSchemaValidator(ctx) + if err != nil { return err } // Init requirements provider - if err := p.requirementsProvider.Initialize(ctx); err != nil { + err = p.requirementsProvider.Initialize(ctx) + if err != nil { return err } p.config.Logger.Debug("Diff processor initialized") + return nil } @@ -114,12 +117,15 @@ func (p *DefaultDiffProcessor) Initialize(ctx context.Context) error { func (p *DefaultDiffProcessor) initializeSchemaValidator(ctx context.Context) error { // If the schema validator implements our interface with LoadCRDs, use it if validator, ok := p.schemaValidator.(*DefaultSchemaValidator); ok { - if err := validator.LoadCRDs(ctx); err != nil { + err := validator.LoadCRDs(ctx) + if err != nil { return errors.Wrap(err, "cannot load CRDs") } + p.config.Logger.Debug("Schema validator initialized with CRDs", "crdCount", len(validator.GetCRDs())) } + return nil } @@ -134,6 +140,7 @@ func (p *DefaultDiffProcessor) PerformDiff(ctx context.Context, stdout io.Writer // Collect all diffs across all resources allDiffs := make(map[string]*dt.ResourceDiff) + var errs []error for _, res := range resources { @@ -154,7 +161,8 @@ func (p *DefaultDiffProcessor) PerformDiff(ctx context.Context, stdout io.Writer // Only render diffs if we found some if len(allDiffs) > 0 { // Render all diffs in a single pass - if err := p.diffRenderer.RenderDiffs(stdout, allDiffs); err != nil { + err := p.diffRenderer.RenderDiffs(stdout, allDiffs) + if err != nil { p.config.Logger.Debug("Failed to render diffs", "error", err) errs = append(errs, errors.Wrap(err, "failed to render diffs")) } @@ -239,6 +247,7 @@ func (p *DefaultDiffProcessor) DiffSingleResource(ctx context.Context, res *un.U // Calculate all diffs p.config.Logger.Debug("Calculating diffs", "resource", resourceID) + diffs, err := p.diffCalculator.CalculateDiffs(ctx, xr, desired) if err != nil { // We don't fail completely if some diffs couldn't be calculated @@ -257,7 +266,9 @@ func (p *DefaultDiffProcessor) DiffSingleResource(ctx context.Context, res *un.U func (p *DefaultDiffProcessor) SanitizeXR(res *un.Unstructured, resourceID string) (*cmp.Unstructured, bool, error) { // Convert the unstructured resource to a composite unstructured for rendering xr := cmp.New() - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.UnstructuredContent(), xr); err != nil { + + err := runtime.DefaultUnstructuredConverter.FromUnstructured(res.UnstructuredContent(), xr) + if err != nil { p.config.Logger.Debug("Failed to convert resource", "resource", resourceID, "error", err) return nil, true, errors.Wrap(err, "cannot convert XR to composite unstructured") } @@ -275,6 +286,7 @@ func (p *DefaultDiffProcessor) SanitizeXR(res *un.Unstructured, resourceID strin xrCopy.SetName(displayName) xr = xrCopy } + return xr, false, nil } @@ -282,7 +294,9 @@ func (p *DefaultDiffProcessor) SanitizeXR(res *un.Unstructured, resourceID strin func mergeUnstructured(dest *un.Unstructured, src *un.Unstructured) (*un.Unstructured, error) { // Start with a deep copy of the rendered resource result := dest.DeepCopy() - if err := mergo.Merge(&result.Object, src.Object, mergo.WithOverride); err != nil { + + err := mergo.Merge(&result.Object, src.Object, mergo.WithOverride) + if err != nil { return nil, errors.Wrap(err, "cannot merge unstructured objects") } @@ -314,8 +328,11 @@ func (p *DefaultDiffProcessor) RenderWithRequirements( // Set up for iterative discovery const maxIterations = 10 // Prevent infinite loops - var lastOutput render.Outputs - var lastRenderErr error + + var ( + lastOutput render.Outputs + lastRenderErr error + ) // Track the number of iterations for logging iteration := 0 @@ -357,6 +374,7 @@ func (p *DefaultDiffProcessor) RenderWithRequirements( "resource", resourceID, "iteration", iteration, "error", renderErr) + return render.Outputs{}, errors.Wrap(renderErr, "cannot render resources") } @@ -373,6 +391,7 @@ func (p *DefaultDiffProcessor) RenderWithRequirements( if len(output.Requirements) == 0 { p.config.Logger.Debug("No more requirements found, discovery complete", "iteration", iteration) + break } @@ -390,11 +409,13 @@ func (p *DefaultDiffProcessor) RenderWithRequirements( if len(additionalResources) == 0 { p.config.Logger.Debug("No new resources found from requirements, discovery complete", "iteration", iteration) + break } // Check if we've already discovered these resources newResourcesFound := false + for _, res := range additionalResources { resourceKey := fmt.Sprintf("%s/%s", res.GetAPIVersion(), res.GetName()) if !discoveredResourcesMap[resourceKey] { @@ -413,6 +434,7 @@ func (p *DefaultDiffProcessor) RenderWithRequirements( if !newResourcesFound { p.config.Logger.Debug("No new unique resources found, discovery complete", "iteration", iteration) + break } diff --git a/cmd/diff/diffprocessor/diff_processor_test.go b/cmd/diff/diffprocessor/diff_processor_test.go index 1558283..8a0afbe 100644 --- a/cmd/diff/diffprocessor/diff_processor_test.go +++ b/cmd/diff/diffprocessor/diff_processor_test.go @@ -462,6 +462,7 @@ func TestDefaultDiffProcessor_PerformDiff(t *testing.T) { if diff := gcmp.Diff(tt.want.Error(), err.Error()); diff != "" { t.Errorf("PerformDiff(...): -want error, +got error:\n%s", diff) } + return } @@ -617,6 +618,7 @@ func TestDefaultDiffProcessor_Initialize(t *testing.T) { if diff := gcmp.Diff(tc.want.Error(), err.Error()); diff != "" { t.Errorf("Initialize(...): -want error, +got error:\n%s", diff) } + return } @@ -654,8 +656,11 @@ func TestDefaultDiffProcessor_RenderWithRequirements(t *testing.T) { } // Create test resources for requirements - const ConfigMap = "ConfigMap" - const ConfigMapName = "config1" + const ( + ConfigMap = "ConfigMap" + ConfigMapName = "config1" + ) + configMap := tu.NewResource("v1", ConfigMap, ConfigMapName).Build() secret := tu.NewResource("v1", "Secret", "secret1").Build() @@ -1054,6 +1059,7 @@ func TestDefaultDiffProcessor_RenderWithRequirements(t *testing.T) { if err == nil { t.Errorf("RenderWithRequirements() expected error but got none") } + return } diff --git a/cmd/diff/diffprocessor/requirements_provider.go b/cmd/diff/diffprocessor/requirements_provider.go index c074309..746c429 100644 --- a/cmd/diff/diffprocessor/requirements_provider.go +++ b/cmd/diff/diffprocessor/requirements_provider.go @@ -61,6 +61,15 @@ func (p *RequirementsProvider) Initialize(ctx context.Context) error { return nil } +// ClearCache clears all cached resources. +func (p *RequirementsProvider) ClearCache() { + p.cacheMutex.Lock() + defer p.cacheMutex.Unlock() + + p.resourceCache = make(map[string]*un.Unstructured) + p.logger.Debug("Resource cache cleared") +} + // cacheResources adds resources to the cache. func (p *RequirementsProvider) cacheResources(resources []*un.Unstructured) { p.cacheMutex.Lock() @@ -78,6 +87,7 @@ func (p *RequirementsProvider) getCachedResource(apiVersion, kind, name string) defer p.cacheMutex.RUnlock() key := fmt.Sprintf("%s/%s/%s", apiVersion, kind, name) + return p.resourceCache[key] } @@ -108,10 +118,13 @@ func (p *RequirementsProvider) ProvideRequirements(ctx context.Context, requirem // processAllSteps processes requirements for all steps without copying protobuf structs. func (p *RequirementsProvider) processAllSteps(ctx context.Context, requirements map[string]v1.Requirements, xrNamespace string) ([]*un.Unstructured, []*un.Unstructured, error) { - var allResources []*un.Unstructured - var newlyFetchedResources []*un.Unstructured + var ( + allResources []*un.Unstructured + newlyFetchedResources []*un.Unstructured + ) // Process each step's requirements + for stepName := range requirements { stepResources, stepNewlyFetched, err := p.processStepSelectors( ctx, @@ -143,15 +156,19 @@ func (p *RequirementsProvider) processStepSelectors(ctx context.Context, stepNam "extraResources", len(extraResources), "total", totalSelectors) - var stepResources []*un.Unstructured - var newlyFetched []*un.Unstructured + var ( + stepResources []*un.Unstructured + newlyFetched []*un.Unstructured + ) // Process Resources selectors + for resourceKey, selector := range resources { res, fetched, err := p.processSelector(ctx, stepName, resourceKey, selector, xrNamespace) if err != nil { return nil, nil, err } + stepResources = append(stepResources, res...) newlyFetched = append(newlyFetched, fetched...) } @@ -162,6 +179,7 @@ func (p *RequirementsProvider) processStepSelectors(ctx context.Context, stepNam if err != nil { return nil, nil, err } + stepResources = append(stepResources, res...) newlyFetched = append(newlyFetched, fetched...) } @@ -175,6 +193,7 @@ func (p *RequirementsProvider) processSelector(ctx context.Context, stepName, re p.logger.Debug("Nil selector in requirements", "step", stepName, "resourceKey", resourceKey) + return nil, nil, nil } @@ -212,6 +231,7 @@ func (p *RequirementsProvider) processSelector(ctx context.Context, stepName, re p.logger.Debug("Unsupported selector type", "step", stepName, "resourceKey", resourceKey) + return nil, nil, nil } } @@ -233,15 +253,6 @@ func parseVersionFromAPIVersion(apiVersion string) string { return version } -// ClearCache clears all cached resources. -func (p *RequirementsProvider) ClearCache() { - p.cacheMutex.Lock() - defer p.cacheMutex.Unlock() - - p.resourceCache = make(map[string]*un.Unstructured) - p.logger.Debug("Resource cache cleared") -} - // resolveNamespace determines the appropriate namespace for a resource based on its scope and selector. func (p *RequirementsProvider) resolveNamespace(ctx context.Context, gvk schema.GroupVersionKind, selector *v1.ResourceSelector, xrNamespace, stepName string) (string, error) { isNamespaced, err := p.client.IsNamespacedResource(ctx, gvk) @@ -256,6 +267,7 @@ func (p *RequirementsProvider) resolveNamespace(ctx context.Context, gvk schema. if selector.GetNamespace() != "" { return selector.GetNamespace(), nil } + return xrNamespace, nil } @@ -269,6 +281,7 @@ func (p *RequirementsProvider) processNameSelector(ctx context.Context, selector "apiVersion", selector.GetApiVersion(), "kind", selector.GetKind(), "name", name) + return []*un.Unstructured{cached}, nil } @@ -321,5 +334,6 @@ func parseAPIVersion(apiVersion string) (string, string) { // Core case: version only (e.g., "v1") version = apiVersion } + return group, version } diff --git a/cmd/diff/diffprocessor/requirements_provider_test.go b/cmd/diff/diffprocessor/requirements_provider_test.go index bb3e19a..92d3976 100644 --- a/cmd/diff/diffprocessor/requirements_provider_test.go +++ b/cmd/diff/diffprocessor/requirements_provider_test.go @@ -263,8 +263,10 @@ func TestRequirementsProvider_ProvideRequirements(t *testing.T) { if err == nil { t.Errorf("ProvideRequirements() expected error but got none") } + return } + if err != nil { t.Fatalf("ProvideRequirements() unexpected error: %v", err) } diff --git a/cmd/diff/diffprocessor/resource_manager.go b/cmd/diff/diffprocessor/resource_manager.go index 1f8e29b..aa2b64f 100644 --- a/cmd/diff/diffprocessor/resource_manager.go +++ b/cmd/diff/diffprocessor/resource_manager.go @@ -69,6 +69,7 @@ func (m *DefaultResourceManager) FetchCurrentObject(ctx context.Context, composi "resourceVersion", current.GetResourceVersion()) m.checkCompositeOwnership(current, composite) + return current, false, nil } @@ -77,6 +78,7 @@ func (m *DefaultResourceManager) FetchCurrentObject(ctx context.Context, composi m.logger.Debug("Error getting resource", "resource", resourceID, "error", err) + return nil, false, err } } @@ -92,6 +94,7 @@ func (m *DefaultResourceManager) FetchCurrentObject(ctx context.Context, composi m.logger.Debug("Error during label-based lookup for resource with generateName (treating as new)", "resource", resourceID, "error", err) + return nil, true, nil } @@ -99,6 +102,7 @@ func (m *DefaultResourceManager) FetchCurrentObject(ctx context.Context, composi m.logger.Debug("Error during label-based lookup", "resource", resourceID, "error", err) + return nil, false, err } @@ -109,6 +113,7 @@ func (m *DefaultResourceManager) FetchCurrentObject(ctx context.Context, composi // We didn't find a matching resource using any strategy m.logger.Debug("No matching resource found", "resource", resourceID) + return nil, true, nil } @@ -119,6 +124,7 @@ func (m *DefaultResourceManager) createResourceID(gvk schema.GroupVersionKind, n if namespace != "" { return fmt.Sprintf("%s/%s/%s", gvk.String(), namespace, name) } + return fmt.Sprintf("%s/%s", gvk.String(), name) } @@ -127,6 +133,7 @@ func (m *DefaultResourceManager) createResourceID(gvk schema.GroupVersionKind, n if namespace != "" { return fmt.Sprintf("%s/%s/%s*", gvk.String(), namespace, generateName) } + return fmt.Sprintf("%s/%s*", gvk.String(), generateName) } @@ -167,6 +174,7 @@ func (m *DefaultResourceManager) lookupByComposite(ctx context.Context, composit if annotations == nil { m.logger.Debug("Resource has no annotations, creating new", "resource", resourceID) + return nil, false, nil } @@ -175,6 +183,7 @@ func (m *DefaultResourceManager) lookupByComposite(ctx context.Context, composit if compResourceName == "" { m.logger.Debug("Resource has no composition-resource-name, creating new", "resource", resourceID) + return nil, false, nil } @@ -190,8 +199,11 @@ func (m *DefaultResourceManager) lookupByComposite(ctx context.Context, composit } // Determine the appropriate label selector based on whether the composite is a claim - var labelSelector metav1.LabelSelector - var lookupName string + var ( + labelSelector metav1.LabelSelector + lookupName string + ) + isCompositeAClaim := m.defClient.IsClaimResource(ctx, composite) if isCompositeAClaim { @@ -231,6 +243,7 @@ func (m *DefaultResourceManager) lookupByComposite(ctx context.Context, composit m.logger.Debug("No resources found with owner labels", "lookupName", lookupName, "isClaimLookup", isCompositeAClaim) + return nil, false, nil } @@ -283,6 +296,7 @@ func (m *DefaultResourceManager) findMatchingResource( m.logger.Debug("Found resource with matching composition name but wrong generateName prefix", "expectedPrefix", generateName, "actualName", resName) + continue } } @@ -291,11 +305,13 @@ func (m *DefaultResourceManager) findMatchingResource( m.logger.Debug("Found resource by label and annotation", "resource", res.GetName(), "compositionResourceName", compResourceName) + return res, true, nil } m.logger.Debug("No matching resource found with composition resource name", "compositionResourceName", compResourceName) + return nil, false, nil } diff --git a/cmd/diff/diffprocessor/resource_manager_test.go b/cmd/diff/diffprocessor/resource_manager_test.go index 75d6da5..c39f4e4 100644 --- a/cmd/diff/diffprocessor/resource_manager_test.go +++ b/cmd/diff/diffprocessor/resource_manager_test.go @@ -334,8 +334,10 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { if err == nil { t.Errorf("FetchCurrentObject() expected error but got none") } + return } + if err != nil { t.Fatalf("FetchCurrentObject() unexpected error: %v", err) } @@ -355,6 +357,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { if current == nil { t.Fatalf("FetchCurrentObject() returned nil current for existing resource") } + if current.GetName() != tt.wantResourceID { t.Errorf("FetchCurrentObject() current.GetName() = %v, want %v", current.GetName(), tt.wantResourceID) @@ -367,6 +370,7 @@ func TestDefaultResourceManager_FetchCurrentObject(t *testing.T) { func TestDefaultResourceManager_UpdateOwnerRefs(t *testing.T) { // Create test resources parentXR := tu.NewResource("example.org/v1", "XR", "parent-xr").Build() + const ParentUID = "parent-uid" parentXR.SetUID(ParentUID) diff --git a/cmd/diff/diffprocessor/schema_validator.go b/cmd/diff/diffprocessor/schema_validator.go index 7d79924..094e041 100644 --- a/cmd/diff/diffprocessor/schema_validator.go +++ b/cmd/diff/diffprocessor/schema_validator.go @@ -71,6 +71,7 @@ func (v *DefaultSchemaValidator) LoadCRDs(ctx context.Context) error { v.crds = crds v.logger.Debug("Loaded CRDs", "count", len(crds)) + return nil } @@ -107,7 +108,8 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U "cachedCRDs", len(v.crds), "resourceCount", len(resources)) - if err := v.EnsureComposedResourceCRDs(ctx, resources); err != nil { + err := v.EnsureComposedResourceCRDs(ctx, resources) + if err != nil { return errors.Wrap(err, "unable to ensure CRDs") } @@ -117,7 +119,9 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U // Validate using the CRD schemas // Use skipSuccessLogs=true to avoid cluttering the output with success messages v.logger.Debug("Performing schema validation", "resourceCount", len(resources)) - if err := validate.SchemaValidation(ctx, resources, v.crds, true, true, loggerWriter); err != nil { + + err = validate.SchemaValidation(ctx, resources, v.crds, true, true, loggerWriter) + if err != nil { return errors.Wrap(err, "schema validation failed") } @@ -125,14 +129,17 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U expectedNamespace := xr.GetNamespace() isClaimRoot := v.defClient.IsClaimResource(ctx, xr) v.logger.Debug("Performing resource scope validation", "resourceCount", len(resources), "expectedNamespace", expectedNamespace, "isClaimRoot", isClaimRoot) + for _, resource := range resources { - if err := v.ValidateScopeConstraints(ctx, resource, expectedNamespace, isClaimRoot); err != nil { + err := v.ValidateScopeConstraints(ctx, resource, expectedNamespace, isClaimRoot) + if err != nil { return errors.Wrapf(err, "resource scope validation failed for %s/%s", resource.GetKind(), resource.GetName()) } } v.logger.Debug("Resources validated successfully") + return nil } @@ -141,6 +148,7 @@ func (v *DefaultSchemaValidator) ValidateResources(ctx context.Context, xr *un.U func (v *DefaultSchemaValidator) EnsureComposedResourceCRDs(ctx context.Context, resources []*un.Unstructured) error { // Create a map of existing CRDs by GVK for quick lookup existingCRDs := make(map[schema.GroupVersionKind]bool) + for _, crd := range v.crds { for _, version := range crd.Spec.Versions { gvk := schema.GroupVersionKind{ @@ -175,6 +183,7 @@ func (v *DefaultSchemaValidator) EnsureComposedResourceCRDs(ctx context.Context, if !v.schemaClient.IsCRDRequired(ctx, gvk) { v.logger.Debug("Skipping built-in resource type, no CRD required", "gvk", gvk.String()) + continue } @@ -184,6 +193,7 @@ func (v *DefaultSchemaValidator) EnsureComposedResourceCRDs(ctx context.Context, v.logger.Debug("CRD not found (continuing)", "gvk", gvk.String(), "error", err) + return errors.New("unable to find CRD for " + gvk.String()) } @@ -193,6 +203,7 @@ func (v *DefaultSchemaValidator) EnsureComposedResourceCRDs(ctx context.Context, v.logger.Debug("Error converting CRD (continuing)", "gvk", gvk.String(), "error", err) + continue } @@ -202,6 +213,7 @@ func (v *DefaultSchemaValidator) EnsureComposedResourceCRDs(ctx context.Context, } v.logger.Debug("Finished ensuring CRDs", "totalCRDs", len(v.crds)) + return nil } @@ -217,6 +229,7 @@ func (v *DefaultSchemaValidator) getResourceScope(ctx context.Context, gvk schem crd.Spec.Names.Kind == gvk.Kind { scope := string(crd.Spec.Scope) v.logger.Debug("Found scope in cached CRDs", "gvk", gvk.String(), "scope", scope) + return scope, nil } } @@ -263,6 +276,7 @@ func (v *DefaultSchemaValidator) ValidateScopeConstraints(ctx context.Context, r if resourceNamespace == "" { return errors.Errorf("namespaced resource %s must have a namespace", resourceID) } + if expectedNamespace != "" && resourceNamespace != expectedNamespace { return errors.Errorf("namespaced resource %s has namespace %s but expected %s (cross-namespace references not supported)", resourceID, resourceNamespace, expectedNamespace) @@ -271,6 +285,7 @@ func (v *DefaultSchemaValidator) ValidateScopeConstraints(ctx context.Context, r if resourceNamespace != "" { return errors.Errorf("cluster-scoped resource %s cannot have a namespace", resourceID) } + if expectedNamespace != "" && !isClaimRoot { return errors.Errorf("namespaced XR cannot own cluster-scoped managed resource %s", resourceID) } diff --git a/cmd/diff/diffprocessor/schema_validator_test.go b/cmd/diff/diffprocessor/schema_validator_test.go index 0b7b018..bcf2ddc 100644 --- a/cmd/diff/diffprocessor/schema_validator_test.go +++ b/cmd/diff/diffprocessor/schema_validator_test.go @@ -197,10 +197,12 @@ func TestDefaultSchemaValidator_ValidateResources(t *testing.T) { t.Errorf("ValidateResources() expected error but got none") return } + if tt.expectedErrMsg != "" && !strings.Contains(err.Error(), tt.expectedErrMsg) { t.Errorf("ValidateResources() error %q doesn't contain expected message %q", err.Error(), tt.expectedErrMsg) } + return } @@ -367,6 +369,7 @@ func TestDefaultSchemaValidator_LoadCRDs(t *testing.T) { if err == nil { t.Errorf("LoadCRDs() expected error but got none") } + return } @@ -432,6 +435,7 @@ func createCRDWithStringField(baseCRD *extv1.CustomResourceDefinition) *extv1.Cu crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["field"] = extv1.JSONSchemaProps{ Type: "string", } + return crd } @@ -441,12 +445,14 @@ func MustToUnstructured(obj interface{}) map[string]interface{} { if err != nil { panic(err) } + return u } // Helper type to track GetXRDs calls. type xrdCountingClient struct { tu.MockDefinitionClient + getXRDsCallCount int } @@ -595,10 +601,12 @@ func TestDefaultSchemaValidator_ValidateScopeConstraints(t *testing.T) { t.Errorf("ValidateScopeConstraints() expected error but got none") return } + if tt.expectedErrMsg != "" && !strings.Contains(err.Error(), tt.expectedErrMsg) { t.Errorf("ValidateScopeConstraints() error %q doesn't contain expected message %q", err.Error(), tt.expectedErrMsg) } + return } diff --git a/cmd/diff/main.go b/cmd/diff/main.go index 9e48740..477fa11 100644 --- a/cmd/diff/main.go +++ b/cmd/diff/main.go @@ -38,6 +38,7 @@ type ( func (v verboseFlag) BeforeApply(ctx *kong.Context) error { //nolint:unparam // BeforeApply requires this signature. logger := logging.NewLogrLogger(zap.New(zap.UseDevMode(true))) ctx.BindTo(logger, (*logging.Logger)(nil)) + return nil } diff --git a/cmd/diff/renderer/diff_formatter.go b/cmd/diff/renderer/diff_formatter.go index 934d649..4dce261 100644 --- a/cmd/diff/renderer/diff_formatter.go +++ b/cmd/diff/renderer/diff_formatter.go @@ -2,6 +2,7 @@ package renderer import ( + "context" "fmt" "strings" @@ -69,6 +70,7 @@ func NewFormatter(compact bool) DiffFormatter { if compact { return &CompactDiffFormatter{} } + return &FullDiffFormatter{} } @@ -129,10 +131,13 @@ func (f *CompactDiffFormatter) Format(diffs []diffmatchpatch.Diff, options DiffO } } - var changeBlocks []changeBlock - var currentBlock *changeBlock + var ( + changeBlocks []changeBlock + currentBlock *changeBlock + ) // Identify all the change blocks + for i, line := range allLines { if line.Type != diffmatchpatch.DiffEqual { // Start a new block if we don't have one @@ -165,6 +170,7 @@ func (f *CompactDiffFormatter) Format(diffs []diffmatchpatch.Diff, options DiffO func (f *CompactDiffFormatter) stringBlocksWithContext(changes []changeBlock, lines []lineItem, opts DiffOptions) string { // Now build compact output with context var builder strings.Builder + contextLines := opts.ContextLines // Keep track of the last line we printed @@ -185,6 +191,7 @@ func (f *CompactDiffFormatter) stringBlocksWithContext(changes []changeBlock, li if contextStart > prevContextEnd { // Add separator builder.WriteString(fmt.Sprintf("%s\n", opts.ChunkSeparator)) + lastPrintedIdx = -1 // Reset to force printing of context lines } else { // Contexts overlap or are adjacent - adjust the start to avoid duplicate lines @@ -197,6 +204,7 @@ func (f *CompactDiffFormatter) stringBlocksWithContext(changes []changeBlock, li if i > lastPrintedIdx { builder.WriteString(lines[i].Formatted) builder.WriteString("\n") + lastPrintedIdx = i } } @@ -205,6 +213,7 @@ func (f *CompactDiffFormatter) stringBlocksWithContext(changes []changeBlock, li for i := block.StartIdx; i <= block.EndIdx; i++ { builder.WriteString(lines[i].Formatted) builder.WriteString("\n") + lastPrintedIdx = i } @@ -213,6 +222,7 @@ func (f *CompactDiffFormatter) stringBlocksWithContext(changes []changeBlock, li for i := block.EndIdx + 1; i < contextEnd; i++ { builder.WriteString(lines[i].Formatted) builder.WriteString("\n") + lastPrintedIdx = i } } @@ -234,7 +244,7 @@ func GetLineDiff(oldText, newText string) []diffmatchpatch.Diff { } // GenerateDiffWithOptions produces a structured diff between two unstructured objects. -func GenerateDiffWithOptions(current, desired *un.Unstructured, logger logging.Logger, _ DiffOptions) (*t.ResourceDiff, error) { +func GenerateDiffWithOptions(_ context.Context, current, desired *un.Unstructured, logger logging.Logger, _ DiffOptions) (*t.ResourceDiff, error) { var diffType t.DiffType // Determine resource identifiers upfront @@ -251,12 +261,15 @@ func GenerateDiffWithOptions(current, desired *un.Unstructured, logger logging.L switch { case current == nil && desired != nil: diffType = t.DiffTypeAdded + logger.Debug("Diff type: Resource is being added", "resource", resourceKey) case current != nil && desired == nil: diffType = t.DiffTypeRemoved + logger.Debug("Diff type: Resource is being removed", "resource", resourceKey) case current != nil: // && desired != nil: diffType = t.DiffTypeModified + logger.Debug("Diff type: Resource is being modified", "resource", resourceKey) default: logger.Debug("Error: both current and desired are nil") @@ -289,11 +302,14 @@ func GenerateDiffWithOptions(current, desired *un.Unstructured, logger logging.L if obj == nil { return "", nil } + clean := cleanupForDiff(obj.DeepCopy(), logger) + yaml, err := sigsyaml.Marshal(clean.Object) if err != nil { return "", err } + return string(yaml), nil } @@ -317,6 +333,7 @@ func GenerateDiffWithOptions(current, desired *un.Unstructured, logger logging.L // Get the line by line diff logger.Debug("Computing line-by-line diff", "resource", resourceKey) + lineDiffs := GetLineDiff(currentStr, desiredStr) if len(lineDiffs) == 0 { @@ -327,9 +344,12 @@ func GenerateDiffWithOptions(current, desired *un.Unstructured, logger logging.L logger.Debug("Diff calculation complete", "resource", resourceKey, "diff_chunks", len(lineDiffs)) // Extract resource kind and name - var name string - var gvk schema.GroupVersionKind + var ( + name string + gvk schema.GroupVersionKind + ) // For removed resources, use current's kind and name + if diffType == t.DiffTypeRemoved { // current != nil name = current.GetName() gvk = current.GroupVersionKind() @@ -401,8 +421,10 @@ func processLines(diff diffmatchpatch.Diff, options DiffOptions) ([]string, bool // formatLine applies the appropriate prefix and color to a single line. func formatLine(line string, diffType diffmatchpatch.Operation, options DiffOptions) string { - var prefix string - var colorStart, colorEnd string + var ( + prefix string + colorStart, colorEnd string + ) switch diffType { case diffmatchpatch.DiffInsert: @@ -424,6 +446,7 @@ func formatLine(line string, diffType diffmatchpatch.Operation, options DiffOpti if options.UseColors && colorStart != "" { return fmt.Sprintf("%s%s%s%s", colorStart, prefix, line, colorEnd) } + return fmt.Sprintf("%s%s", prefix, line) } @@ -449,6 +472,7 @@ func cleanupForDiff(obj *un.Unstructured, logger logging.Logger) *un.Unstructure // This is a display name we added for diffing purposes - remove it // since we only added it for diffing but don't want it to show in the actual diff delete(metadata, "name") + modifications = append(modifications, fmt.Sprintf("removed display name %q", name)) // Also normalize generateName by removing any "(generated)" suffix @@ -478,6 +502,7 @@ func cleanupForDiff(obj *un.Unstructured, logger logging.Logger) *un.Unstructure // Track which fields were actually removed for debugging var removedFields []string + for _, field := range fieldsToRemove { if _, exists := metadata[field]; exists { delete(metadata, field) @@ -499,6 +524,7 @@ func cleanupForDiff(obj *un.Unstructured, logger logging.Logger) *un.Unstructure if found && spec != nil { if _, exists := spec["resourceRefs"]; exists { delete(spec, "resourceRefs") + modifications = append(modifications, "resourceRefs from spec") } @@ -509,6 +535,7 @@ func cleanupForDiff(obj *un.Unstructured, logger logging.Logger) *un.Unstructure if found && crossplane != nil { if _, exists := crossplane["resourceRefs"]; exists { delete(crossplane, "resourceRefs") + modifications = append(modifications, "resourceRefs from spec.crossplane") } @@ -526,6 +553,7 @@ func cleanupForDiff(obj *un.Unstructured, logger logging.Logger) *un.Unstructure // Remove status field as we're focused on spec changes if _, exists := obj.Object["status"]; exists { delete(obj.Object, "status") + modifications = append(modifications, "status field") } diff --git a/cmd/diff/renderer/diff_formatter_test.go b/cmd/diff/renderer/diff_formatter_test.go index b353b0e..3e4753e 100644 --- a/cmd/diff/renderer/diff_formatter_test.go +++ b/cmd/diff/renderer/diff_formatter_test.go @@ -106,12 +106,13 @@ func TestGenerateDiffWithOptions(t *testing.T) { } for name, tt := range tests { t.Run(name, func(t *testing.T) { - diff, err := GenerateDiffWithOptions(tt.current, tt.desired, tu.TestLogger(t, false), tt.options) + diff, err := GenerateDiffWithOptions(t.Context(), tt.current, tt.desired, tu.TestLogger(t, false), tt.options) if tt.wantErr { if err == nil { t.Errorf("GenerateDiffWithOptions() expected error but got none") } + return } @@ -272,6 +273,7 @@ func TestFormatDiff(t *testing.T) { if expected == "" { continue } + if !strings.Contains(result, expected) { t.Errorf("FormatDiff() result missing expected content: %q", expected) } @@ -282,6 +284,7 @@ func TestFormatDiff(t *testing.T) { if excluded == "" { continue } + if strings.Contains(result, excluded) { t.Errorf("FormatDiff() result contains unexpected content: %q", excluded) } diff --git a/cmd/diff/renderer/diff_renderer.go b/cmd/diff/renderer/diff_renderer.go index 7f652f6..5ed10b3 100644 --- a/cmd/diff/renderer/diff_renderer.go +++ b/cmd/diff/renderer/diff_renderer.go @@ -90,6 +90,7 @@ func (r *DefaultDiffRenderer) RenderDiffs(stdout io.Writer, diffs map[string]*dt // Format the diff header based on the diff type var header string + switch diff.DiffType { case dt.DiffTypeAdded: header = fmt.Sprintf("+++ %s", resourceID) @@ -111,6 +112,7 @@ func (r *DefaultDiffRenderer) RenderDiffs(stdout io.Writer, diffs map[string]*dt r.logger.Debug("Error writing diff to output", "resource", resourceID, "error", err) return errors.Wrap(err, "failed to write diff to output") } + outputCount++ } else { r.logger.Debug("Empty diff content, skipping output", "resource", resourceID) @@ -132,9 +134,11 @@ func (r *DefaultDiffRenderer) RenderDiffs(stdout io.Writer, diffs map[string]*dt if addedCount > 0 { summary.WriteString(fmt.Sprintf("%d added, ", addedCount)) } + if modifiedCount > 0 { summary.WriteString(fmt.Sprintf("%d modified, ", modifiedCount)) } + if removedCount > 0 { summary.WriteString(fmt.Sprintf("%d removed, ", removedCount)) } diff --git a/cmd/diff/renderer/diff_renderer_test.go b/cmd/diff/renderer/diff_renderer_test.go index eb73bbb..8d05ab8 100644 --- a/cmd/diff/renderer/diff_renderer_test.go +++ b/cmd/diff/renderer/diff_renderer_test.go @@ -245,9 +245,11 @@ func TestGetLineDiff(t *testing.T) { // Check that we have the expected diff types if len(result) != len(tt.expected) { t.Errorf("GetLineDiff() returned %d diffs, want %d", len(result), len(tt.expected)) + for i, diff := range result { t.Logf("Diff %d: Type=%s, Text=%q", i, diff.Type, diff.Text) } + return } diff --git a/cmd/diff/renderer/types/types.go b/cmd/diff/renderer/types/types.go index 1ddbbca..4bab804 100644 --- a/cmd/diff/renderer/types/types.go +++ b/cmd/diff/renderer/types/types.go @@ -1,5 +1,5 @@ // Package types provides types used in the renderer in order to facilitate code reuse in test -package types +package types //nolint:revive // types is an appropriate name for a types package import ( "fmt" diff --git a/cmd/diff/testutils/mock_builder.go b/cmd/diff/testutils/mock_builder.go index 9959c39..7f593ae 100644 --- a/cmd/diff/testutils/mock_builder.go +++ b/cmd/diff/testutils/mock_builder.go @@ -61,6 +61,7 @@ func (b *MockResourceClientBuilder) WithFoundGVKs(gvks []schema.GroupVersionKind b.mock.GetGVKsForGroupKindFn = func(_ context.Context, _, _ string) ([]schema.GroupVersionKind, error) { return gvks, nil } + return b } @@ -69,6 +70,7 @@ func (b *MockResourceClientBuilder) WithoutFoundGVKs(err string) *MockResourceCl b.mock.GetGVKsForGroupKindFn = func(_ context.Context, _, _ string) ([]schema.GroupVersionKind, error) { return nil, errors.New(err) } + return b } @@ -95,6 +97,7 @@ func (b *MockResourceClientBuilder) WithResourcesExist(resources ...*un.Unstruct if res, found := resourceMap[key]; found { return res, nil } + return nil, errors.Errorf("resource %q not found", name) }) } @@ -146,6 +149,7 @@ func (b *MockResourceClientBuilder) WithResourcesFoundByLabel(resources []*un.Un if labelValue, exists := selector.MatchLabels[label]; exists && labelValue == value { return resources, nil } + return []*un.Unstructured{}, nil }) } @@ -236,6 +240,7 @@ func (b *MockSchemaClientBuilder) WithSuccessfulCRDFetch(crd *un.Unstructured) * if crd.GetKind() != "CustomResourceDefinition" { return nil, errors.Errorf("setup error: desired return from GetCRD isn't a CRD but a %s", crd.GetKind()) } + return crd, nil }) } @@ -413,6 +418,7 @@ func (b *MockCompositionClientBuilder) WithSuccessfulInitialize() *MockCompositi b.mock.InitializeFn = func(context.Context) error { return nil } + return b } @@ -421,6 +427,7 @@ func (b *MockCompositionClientBuilder) WithFailedInitialize(errMsg string) *Mock b.mock.InitializeFn = func(context.Context) error { return errors.New(errMsg) } + return b } @@ -484,6 +491,7 @@ func (b *MockFunctionClientBuilder) WithSuccessfulInitialize() *MockFunctionClie b.mock.InitializeFn = func(context.Context) error { return nil } + return b } @@ -492,6 +500,7 @@ func (b *MockFunctionClientBuilder) WithFailedInitialize(errMsg string) *MockFun b.mock.InitializeFn = func(context.Context) error { return errors.New(errMsg) } + return b } @@ -549,6 +558,7 @@ func (b *MockEnvironmentClientBuilder) WithSuccessfulInitialize() *MockEnvironme b.mock.InitializeFn = func(context.Context) error { return nil } + return b } @@ -557,6 +567,7 @@ func (b *MockEnvironmentClientBuilder) WithFailedInitialize(errMsg string) *Mock b.mock.InitializeFn = func(context.Context) error { return errors.New(errMsg) } + return b } @@ -607,6 +618,7 @@ func (b *MockDefinitionClientBuilder) WithSuccessfulInitialize() *MockDefinition b.mock.InitializeFn = func(context.Context) error { return nil } + return b } @@ -615,6 +627,7 @@ func (b *MockDefinitionClientBuilder) WithFailedInitialize(errMsg string) *MockD b.mock.InitializeFn = func(context.Context) error { return errors.New(errMsg) } + return b } @@ -727,6 +740,7 @@ func (b *MockResourceTreeClientBuilder) WithSuccessfulInitialize() *MockResource b.mock.InitializeFn = func(context.Context) error { return nil } + return b } @@ -735,6 +749,7 @@ func (b *MockResourceTreeClientBuilder) WithFailedInitialize(errMsg string) *Moc b.mock.InitializeFn = func(context.Context) error { return errors.New(errMsg) } + return b } @@ -858,6 +873,7 @@ func (b *DiffProcessorBuilder) WithDiffOutput(output string) *DiffProcessorBuild if stdout != nil { _, _ = io.WriteString(stdout, output) } + return nil }) } @@ -907,6 +923,7 @@ func (b *ResourceBuilder) InNamespace(namespace string) *ResourceBuilder { if namespace != "" { b.resource.SetNamespace(namespace) } + return b } @@ -915,6 +932,7 @@ func (b *ResourceBuilder) WithGenerateName(generateName string) *ResourceBuilder if generateName != "" { b.resource.SetGenerateName(generateName) } + return b } @@ -923,6 +941,7 @@ func (b *ResourceBuilder) WithLabels(labels map[string]string) *ResourceBuilder if len(labels) > 0 { b.resource.SetLabels(labels) } + return b } @@ -931,6 +950,7 @@ func (b *ResourceBuilder) WithAnnotations(annotations map[string]string) *Resour if len(annotations) > 0 { b.resource.SetAnnotations(annotations) } + return b } @@ -939,6 +959,7 @@ func (b *ResourceBuilder) WithSpec(spec map[string]interface{}) *ResourceBuilder if len(spec) > 0 { _ = un.SetNestedMap(b.resource.Object, spec, "spec") } + return b } @@ -948,8 +969,10 @@ func (b *ResourceBuilder) WithSpecField(name string, value interface{}) *Resourc if spec == nil { spec = map[string]interface{}{} } + spec[name] = value _ = un.SetNestedMap(b.resource.Object, spec, "spec") + return b } @@ -958,6 +981,7 @@ func (b *ResourceBuilder) WithStatus(status map[string]interface{}) *ResourceBui if len(status) > 0 { _ = un.SetNestedMap(b.resource.Object, status, "status") } + return b } @@ -967,8 +991,10 @@ func (b *ResourceBuilder) WithStatusField(name string, value interface{}) *Resou if status == nil { status = map[string]interface{}{} } + status[name] = value _ = un.SetNestedMap(b.resource.Object, status, "status") + return b } @@ -1001,6 +1027,7 @@ func (b *ResourceBuilder) WithCompositeOwner(owner string) *ResourceBuilder { if labels == nil { labels = map[string]string{} } + labels["crossplane.io/composite"] = owner b.resource.SetLabels(labels) @@ -1013,6 +1040,7 @@ func (b *ResourceBuilder) WithCompositionResourceName(name string) *ResourceBuil if annotations == nil { annotations = map[string]string{} } + annotations["crossplane.io/composition-resource-name"] = name b.resource.SetAnnotations(annotations) @@ -1028,6 +1056,7 @@ func (b *ResourceBuilder) Build() *un.Unstructured { func (b *ResourceBuilder) BuildUComposite() *cmp.Unstructured { built := &cmp.Unstructured{} built.SetUnstructuredContent(b.Build().UnstructuredContent()) + return built } @@ -1035,6 +1064,7 @@ func (b *ResourceBuilder) BuildUComposite() *cmp.Unstructured { func (b *ResourceBuilder) BuildUComposed() *cpd.Unstructured { built := &cpd.Unstructured{} built.SetUnstructuredContent(b.Build().UnstructuredContent()) + return built } @@ -1073,6 +1103,7 @@ func (b *CompositionBuilder) WithCompositeTypeRef(apiVersion, kind string) *Comp APIVersion: apiVersion, Kind: kind, } + return b } @@ -1085,6 +1116,7 @@ func (b *CompositionBuilder) WithPipelineMode() *CompositionBuilder { // WithPipelineStep adds a pipeline step to the composition. func (b *CompositionBuilder) WithPipelineStep(step, functionName string, input map[string]interface{}) *CompositionBuilder { var rawInput *runtime.RawExtension + if input != nil { // Properly serialize the map to JSON bytes jsonBytes, err := json.Marshal(input) @@ -1100,6 +1132,7 @@ func (b *CompositionBuilder) WithPipelineStep(step, functionName string, input m FunctionRef: xpextv1.FunctionReference{Name: functionName}, Input: rawInput, }) + return b } diff --git a/cmd/diff/testutils/mocks.go b/cmd/diff/testutils/mocks.go index ce6a93f..86f8161 100644 --- a/cmd/diff/testutils/mocks.go +++ b/cmd/diff/testutils/mocks.go @@ -63,6 +63,7 @@ func (m *MockNamespaceableResourceInterface) Namespace(namespace string) dynamic if m.NamespaceFn != nil { return m.NamespaceFn(namespace) } + return &MockResourceInterface{ GetFn: m.GetFn, ListFn: m.ListFn, @@ -78,6 +79,7 @@ func (m *MockNamespaceableResourceInterface) Create(ctx context.Context, obj *un if m.CreateFn != nil { return m.CreateFn(ctx, obj, options, subresources...) } + return nil, nil } @@ -86,6 +88,7 @@ func (m *MockNamespaceableResourceInterface) Update(ctx context.Context, obj *un if m.UpdateFn != nil { return m.UpdateFn(ctx, obj, options, subresources...) } + return nil, nil } @@ -99,6 +102,7 @@ func (m *MockNamespaceableResourceInterface) Delete(ctx context.Context, name st if m.DeleteFn != nil { return m.DeleteFn(ctx, name, options, subresources...) } + return nil } @@ -112,6 +116,7 @@ func (m *MockNamespaceableResourceInterface) Get(ctx context.Context, name strin if m.GetFn != nil { return m.GetFn(ctx, name, options, subresources...) } + return nil, nil } @@ -120,6 +125,7 @@ func (m *MockNamespaceableResourceInterface) List(ctx context.Context, opts meta if m.ListFn != nil { return m.ListFn(ctx, opts) } + return nil, nil } @@ -133,6 +139,7 @@ func (m *MockNamespaceableResourceInterface) Patch(ctx context.Context, name str if m.PatchFn != nil { return m.PatchFn(ctx, name, pt, data, options, subresources...) } + return nil, nil } @@ -165,6 +172,7 @@ func (m *MockResourceInterface) Create(ctx context.Context, obj *un.Unstructured if m.CreateFn != nil { return m.CreateFn(ctx, obj, options, subresources...) } + return nil, nil } @@ -173,6 +181,7 @@ func (m *MockResourceInterface) Update(ctx context.Context, obj *un.Unstructured if m.UpdateFn != nil { return m.UpdateFn(ctx, obj, options, subresources...) } + return nil, nil } @@ -186,6 +195,7 @@ func (m *MockResourceInterface) Delete(ctx context.Context, name string, options if m.DeleteFn != nil { return m.DeleteFn(ctx, name, options, subresources...) } + return nil } @@ -199,6 +209,7 @@ func (m *MockResourceInterface) Get(ctx context.Context, name string, options me if m.GetFn != nil { return m.GetFn(ctx, name, options, subresources...) } + return nil, nil } @@ -207,6 +218,7 @@ func (m *MockResourceInterface) List(ctx context.Context, opts metav1.ListOption if m.ListFn != nil { return m.ListFn(ctx, opts) } + return nil, nil } @@ -220,6 +232,7 @@ func (m *MockResourceInterface) Patch(ctx context.Context, name string, pt types if m.PatchFn != nil { return m.PatchFn(ctx, name, pt, data, options, subresources...) } + return nil, nil } @@ -249,6 +262,7 @@ func (m *MockDiffProcessor) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -257,6 +271,7 @@ func (m *MockDiffProcessor) PerformDiff(ctx context.Context, stdout io.Writer, r if m.PerformDiffFn != nil { return m.PerformDiffFn(stdout, ctx, resources) } + return nil } @@ -275,6 +290,7 @@ func (m *MockSchemaValidator) ValidateResources(ctx context.Context, xr *un.Unst if m.ValidateResourcesFn != nil { return m.ValidateResourcesFn(ctx, xr, composed) } + return nil } @@ -288,6 +304,7 @@ func (m *MockSchemaValidator) ValidateScopeConstraints(ctx context.Context, reso if m.ValidateScopeConstraintsFn != nil { return m.ValidateScopeConstraintsFn(ctx, resource, expectedNamespace, isClaimRoot) } + return nil } @@ -315,6 +332,7 @@ func (m *MockResourceClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -323,6 +341,7 @@ func (m *MockResourceClient) GetResource(ctx context.Context, gvk schema.GroupVe if m.GetResourceFn != nil { return m.GetResourceFn(ctx, gvk, namespace, name) } + return nil, errors.New("GetResource not implemented") } @@ -331,6 +350,7 @@ func (m *MockResourceClient) ListResources(ctx context.Context, gvk schema.Group if m.ListResourcesFn != nil { return m.ListResourcesFn(ctx, gvk, namespace) } + return nil, errors.New("ListResources not implemented") } @@ -339,6 +359,7 @@ func (m *MockResourceClient) GetResourcesByLabel(ctx context.Context, gvk schema if m.GetResourcesByLabelFn != nil { return m.GetResourcesByLabelFn(ctx, gvk, namespace, sel) } + return nil, errors.New("GetResourcesByLabel not implemented") } @@ -347,6 +368,7 @@ func (m *MockResourceClient) GetAllResourcesByLabels(ctx context.Context, gvks [ if m.GetAllResourcesByLabelsFn != nil { return m.GetAllResourcesByLabelsFn(ctx, gvks, selectors) } + return nil, errors.New("GetAllResourcesByLabels not implemented") } @@ -355,6 +377,7 @@ func (m *MockResourceClient) GetGVKsForGroupKind(ctx context.Context, group, kin if m.GetGVKsForGroupKindFn != nil { return m.GetGVKsForGroupKindFn(ctx, group, kind) } + return nil, errors.New("GetGVKsForGroupKind not implemented") } @@ -363,6 +386,7 @@ func (m *MockResourceClient) IsNamespacedResource(ctx context.Context, gvk schem if m.IsNamespacedResourceFn != nil { return m.IsNamespacedResourceFn(ctx, gvk) } + return false, errors.Errorf("IsNamespacedResource not implemented for %s in mock", gvk.String()) } @@ -379,6 +403,7 @@ func (m *MockSchemaClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -387,6 +412,7 @@ func (m *MockSchemaClient) GetCRD(ctx context.Context, gvk schema.GroupVersionKi if m.GetCRDFn != nil { return m.GetCRDFn(ctx, gvk) } + return nil, errors.New("GetCRD not implemented") } @@ -395,6 +421,7 @@ func (m *MockSchemaClient) IsCRDRequired(ctx context.Context, gvk schema.GroupVe if m.IsCRDRequiredFn != nil { return m.IsCRDRequiredFn(ctx, gvk) } + return true // Default to true } @@ -403,6 +430,7 @@ func (m *MockSchemaClient) ValidateResource(ctx context.Context, resource *un.Un if m.ValidateResourceFn != nil { return m.ValidateResourceFn(ctx, resource) } + return nil } @@ -418,6 +446,7 @@ func (m *MockApplyClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -426,6 +455,7 @@ func (m *MockApplyClient) Apply(ctx context.Context, obj *un.Unstructured) (*un. if m.ApplyFn != nil { return m.ApplyFn(ctx, obj) } + return nil, errors.New("Apply not implemented") } @@ -434,6 +464,7 @@ func (m *MockApplyClient) DryRunApply(ctx context.Context, obj *un.Unstructured) if m.DryRunApplyFn != nil { return m.DryRunApplyFn(ctx, obj) } + return nil, errors.New("DryRunApply not implemented") } @@ -448,6 +479,7 @@ func (m *MockTypeConverter) GVKToGVR(ctx context.Context, gvk schema.GroupVersio if m.GVKToGVRFn != nil { return m.GVKToGVRFn(ctx, gvk) } + return schema.GroupVersionResource{}, errors.New("GVKToGVR not implemented") } @@ -456,6 +488,7 @@ func (m *MockTypeConverter) GetResourceNameForGVK(ctx context.Context, gvk schem if m.GetResourceNameForGVKFn != nil { return m.GetResourceNameForGVKFn(ctx, gvk) } + return "", errors.New("GetResourceNameForGVK not implemented") } @@ -480,6 +513,7 @@ func (m *MockCompositionClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -488,6 +522,7 @@ func (m *MockCompositionClient) FindMatchingComposition(ctx context.Context, res if m.FindMatchingCompositionFn != nil { return m.FindMatchingCompositionFn(ctx, res) } + return nil, errors.New("FindMatchingComposition not implemented") } @@ -496,6 +531,7 @@ func (m *MockCompositionClient) ListCompositions(ctx context.Context) ([]*xpextv if m.ListCompositionsFn != nil { return m.ListCompositionsFn(ctx) } + return nil, errors.New("ListCompositions not implemented") } @@ -504,6 +540,7 @@ func (m *MockCompositionClient) GetComposition(ctx context.Context, name string) if m.GetCompositionFn != nil { return m.GetCompositionFn(ctx, name) } + return nil, errors.New("GetComposition not implemented") } @@ -519,6 +556,7 @@ func (m *MockFunctionClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -527,6 +565,7 @@ func (m *MockFunctionClient) GetFunctionsFromPipeline(comp *xpextv1.Composition) if m.GetFunctionsFromPipelineFn != nil { return m.GetFunctionsFromPipelineFn(comp) } + return nil, errors.New("GetFunctionsFromPipeline not implemented") } @@ -535,6 +574,7 @@ func (m *MockFunctionClient) ListFunctions(ctx context.Context) ([]pkgv1.Functio if m.ListFunctionsFn != nil { return m.ListFunctionsFn(ctx) } + return nil, errors.New("ListFunctions not implemented") } @@ -550,6 +590,7 @@ func (m *MockEnvironmentClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -558,6 +599,7 @@ func (m *MockEnvironmentClient) GetEnvironmentConfigs(ctx context.Context) ([]*u if m.GetEnvironmentConfigsFn != nil { return m.GetEnvironmentConfigsFn(ctx) } + return nil, errors.New("GetEnvironmentConfigs not implemented") } @@ -566,6 +608,7 @@ func (m *MockEnvironmentClient) GetEnvironmentConfig(ctx context.Context, name s if m.GetEnvironmentConfigFn != nil { return m.GetEnvironmentConfigFn(ctx, name) } + return nil, errors.New("GetEnvironmentConfig not implemented") } @@ -583,6 +626,7 @@ func (m *MockDefinitionClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -591,6 +635,7 @@ func (m *MockDefinitionClient) GetXRDs(ctx context.Context) ([]*un.Unstructured, if m.GetXRDsFn != nil { return m.GetXRDsFn(ctx) } + return nil, errors.New("GetXRDs not implemented") } @@ -599,6 +644,7 @@ func (m *MockDefinitionClient) GetXRDForClaim(ctx context.Context, gvk schema.Gr if m.GetXRDForClaimFn != nil { return m.GetXRDForClaimFn(ctx, gvk) } + return nil, errors.New("GetXRDForClaim not implemented") } @@ -607,6 +653,7 @@ func (m *MockDefinitionClient) GetXRDForXR(ctx context.Context, gvk schema.Group if m.GetXRDForXRFn != nil { return m.GetXRDForXRFn(ctx, gvk) } + return nil, errors.New("GetXRDForXR not implemented") } @@ -615,6 +662,7 @@ func (m *MockDefinitionClient) IsClaimResource(ctx context.Context, resource *un if m.IsClaimResourceFn != nil { return m.IsClaimResourceFn(ctx, resource) } + return false } @@ -629,6 +677,7 @@ func (m *MockResourceTreeClient) Initialize(ctx context.Context) error { if m.InitializeFn != nil { return m.InitializeFn(ctx) } + return nil } @@ -637,6 +686,7 @@ func (m *MockResourceTreeClient) GetResourceTree(ctx context.Context, root *un.U if m.GetResourceTreeFn != nil { return m.GetResourceTreeFn(ctx, root) } + return nil, errors.New("GetResourceTree not implemented") } @@ -654,6 +704,7 @@ func (m *MockDiffCalculator) CalculateDiff(ctx context.Context, composite *un.Un if m.CalculateDiffFn != nil { return m.CalculateDiffFn(ctx, composite, desired) } + return nil, nil } @@ -662,6 +713,7 @@ func (m *MockDiffCalculator) CalculateDiffs(ctx context.Context, xr *cmp.Unstruc if m.CalculateDiffsFn != nil { return m.CalculateDiffsFn(ctx, xr, desired) } + return nil, nil } @@ -670,6 +722,7 @@ func (m *MockDiffCalculator) CalculateRemovedResourceDiffs(ctx context.Context, if m.CalculateRemovedResourceDiffsFn != nil { return m.CalculateRemovedResourceDiffsFn(ctx, xr, renderedResources) } + return nil, nil } @@ -683,5 +736,6 @@ func (m *MockDiffRenderer) RenderDiffs(w io.Writer, diffs map[string]*dt.Resourc if m.RenderDiffsFn != nil { return m.RenderDiffsFn(w, diffs) } + return nil } diff --git a/cmd/diff/testutils/utils.go b/cmd/diff/testutils/utils.go index b79574e..91f7e95 100644 --- a/cmd/diff/testutils/utils.go +++ b/cmd/diff/testutils/utils.go @@ -37,6 +37,7 @@ func Green(input string) string { coloredLines = append(coloredLines, "") continue } + coloredLines = append(coloredLines, ColorGreen+line+ColorReset) } @@ -55,6 +56,7 @@ func Red(input string) string { coloredLines = append(coloredLines, "") continue } + coloredLines = append(coloredLines, ColorRed+line+ColorReset) } @@ -100,6 +102,7 @@ func TestLogger(t *testing.T, verbose bool) logging.Logger { if verbose { verbosity = 1 } + return logging.NewLogrLogger(testr.NewWithOptions(t, testr.Options{Verbosity: verbosity})) } @@ -119,5 +122,6 @@ func CreateFakeDiscoveryClient(resources map[string][]metav1.APIResource) discov } fakeDiscovery.Resources = apiResourceLists + return fakeDiscovery } diff --git a/test/e2e/diff_test.go b/test/e2e/diff_test.go index 11aca0b..98799b4 100644 --- a/test/e2e/diff_test.go +++ b/test/e2e/diff_test.go @@ -69,11 +69,13 @@ func RunDiff(t *testing.T, c *envconf.Config, binPath string, resourcePaths ...s args := append([]string{"--verbose", "diff", "--timeout=2m", "-n", namespace}, resourcePaths...) t.Logf("Running command: %s %s", binPath, strings.Join(args, " ")) cmd := exec.Command(binPath, args...) + cmd.Env = append(os.Environ(), "KUBECONFIG="+c.KubeconfigFile()) t.Logf("ENV: %s KUBECONFIG=%s", binPath, c.KubeconfigFile()) // Capture standard output and error var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout cmd.Stderr = &stderr @@ -395,8 +397,10 @@ func normalizeLine(line string) string { // parseStringContent converts a string to raw and normalized lines. func parseStringContent(content string) ([]string, []string) { - var rawLines []string - var normalizedLines []string + var ( + rawLines []string + normalizedLines []string + ) scanner := bufio.NewScanner(strings.NewReader(content)) for scanner.Scan() { @@ -432,25 +436,31 @@ func assertDiffMatchesFile(t *testing.T, actual, expectedSource, log string) { } failed := false + for i := range maxLines { if i >= len(expectedNormalized) { t.Errorf("Line %d: Extra line in output: %s", i+1, makeStringReadable(actualRaw[i])) + failed = true + continue } + if i >= len(actualNormalized) { t.Errorf("Line %d: Missing line in output: %s", i+1, makeStringReadable(expectedRaw[i])) + failed = true + continue } + if expectedNormalized[i] != actualNormalized[i] { // ignore white space at end of lines // if strings.TrimRight(expectedNormalized[i], " ") == strings.TrimRight(actualNormalized[i], " ") { // continue //} - rawExpected := "" if i < len(expectedRaw) { rawExpected = expectedRaw[i] @@ -470,6 +480,7 @@ func assertDiffMatchesFile(t *testing.T, actual, expectedSource, log string) { actualNormalized[i], makeStringReadable(rawExpected), makeStringReadable(rawActual)) + failed = true } } @@ -487,6 +498,7 @@ func assertDiffMatchesFile(t *testing.T, actual, expectedSource, log string) { // (including ANSI escape codes) are converted to visible escape sequences. func makeStringReadable(s string) string { var result strings.Builder + for _, r := range s { switch { case r == '\x1b': @@ -505,5 +517,6 @@ func makeStringReadable(s string) string { result.WriteRune(r) } } + return result.String() } diff --git a/test/e2e/main_test.go b/test/e2e/main_test.go index 64d5f0a..f5a5eb3 100644 --- a/test/e2e/main_test.go +++ b/test/e2e/main_test.go @@ -90,8 +90,10 @@ func TestMain(m *testing.M) { }), ) - var setup []env.Func - var finish []env.Func + var ( + setup []env.Func + finish []env.Func + ) if environment.IsKindCluster() { setup = append(setup, envfuncs.CreateClusterWithConfig( @@ -149,6 +151,7 @@ func TestMain(m *testing.M) { if _, exists := feature.Labels()[config.LabelTestSuite]; !exists { t.Fatalf("Feature %q does not have the required %q label set", feature.Name(), config.LabelTestSuite) } + return ctx, nil })