Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add tests for kustomize's sortOptions configuration #1615

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

lburgazzoli
Copy link
Contributor

@lburgazzoli lburgazzoli commented Feb 5, 2025

The Kustomize rendering engine embedded in the operator, does not
enforce any particular order, but relies on the order of the manifests
provided, however, Kustomize has options to change the strategy used
to sort resources at the end of the Kustomize build see:

This commit adds some test to ensure the sortOrder configuration is
honored by the operator's own Kustomize engine.

Description

https://issues.redhat.com/browse/RHOAIENG-19091

How Has This Been Tested?

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@openshift-ci openshift-ci bot requested review from ugiordan and zdtsw February 5, 2025 09:50
@lburgazzoli
Copy link
Contributor Author

This probably can supersede #1600

@zdtsw
Copy link
Member

zdtsw commented Feb 5, 2025

This probably can supersede #1600

so the plan is to ask component teams to set fifo in their kustomization as the solution for ordering?

@lburgazzoli
Copy link
Contributor Author

so the plan is to ask component teams to set fifo in their kustomization as the solution for ordering?

fifo is what we have as today, they have the possibility to use the strategy that fits better for them, eventually they can define a specific ordering without having to change any odh operator code.

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 20.35%. Comparing base (422544b) to head (a4da554).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/utils/test/matchers/jq/jq_support.go 66.66% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1615      +/-   ##
==========================================
+ Coverage   20.28%   20.35%   +0.07%     
==========================================
  Files         163      163              
  Lines       11137    11155      +18     
==========================================
+ Hits         2259     2271      +12     
- Misses       8638     8644       +6     
  Partials      240      240              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can optimize it like this:

package jq_test

import (
	"encoding/json"
	"testing"

	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq"

	. "github.com/onsi/gomega"
)

// TestMatcher verifies the jq.Match function against various JSON strings.
// It ensures correct behavior for simple key-value matches, array value checks, 
// handling of null values, and nested object conditions.
func TestMatcher(t *testing.T) {
	t.Parallel()

	g := NewWithT(t)

	// Define multiple test cases for jq matching
	testCases := []struct {
		input    string
		jqQuery  string
		expected bool
	}{
		{
			// Test matching a simple value
			input:    `{"a":1}`,
			jqQuery:  `.a == 1`,
			expected: true,
		},
		{
			// Test when the value doesn't match
			input:    `{"a":1}`,
			jqQuery:  `.a == 2`,
			expected: false,
		},
		{
			// Test array with values matching
			input:    `{"Values":[ "foo" ]}`,
			jqQuery:  `.Values | if . then any(. == "foo") else false end`,
			expected: true,
		},
		{
			// Test array with non-matching value
			input:    `{"Values":[ "foo" ]}`,
			jqQuery:  `.Values | if . then any(. == "bar") else false end`,
			expected: false,
		},
		{
			// Test when the value is null
			input:    `{"Values": null}`,
			jqQuery:  `.Values | if . then any(. == "foo") else false end`,
			expected: false,
		},
		{
			// Test multiple matching conditions
			input:    `{ "status": { "foo": { "bar": "fr", "baz": "fb" } } }`,
			jqQuery: `(.status.foo.bar == "fr") and (.status.foo.baz == "fb")`
			expected: true,
		},
	}

	// Run the test cases
	for _, tc := range testCases {
		t.Run(tc.jqQuery, func(t *testing.T) {
			// Assert based on the expected result
			if tc.expected {
				g.Expect(tc.input).Should(jq.Match(tc.jqQuery))
			} else {
				g.Expect(tc.input).ShouldNot(jq.Match(tc.jqQuery))
			}
		})
	}
}
// TestMatcherWithType validates jq.Match against different Go data types such as maps and structs.
// It ensures that jq.Match works correctly after serializing Go types to JSON.
func TestMatcherWithType(t *testing.T) {
	t.Parallel()

	g := NewWithT(t)

	// Define multiple test cases for jq matching
	testCases := []struct {
		name     string
		input    interface{}
		jqQuery  string
		useTransform bool
	}{
		{
			name:        "Simple map match",
			input:       map[string]any{"a": 1},
			jqQuery:     `.a == 1`,
			useTransform: false,
		},
		{
			name:        "Map match with JSON transformation",
			input:       map[string]any{"a": 1},
			jqQuery:     `.a == 1`,
			useTransform: true,
		},
		{
			name: "Nested map match with JSON transformation",
			input: map[string]any{
				"status": map[string]any{
					"foo": map[string]any{
						"bar": "fr",
						"baz": "fb",
					},
				},
			},
			jqQuery: `(.status.foo.bar == "fr") and (.status.foo.baz == "fb"`),
			useTransform: true,
		},
		{
			name:        "Struct match with JSON transformation",
			input:       struct { A int `json:"a"` }{A: 1},
			jqQuery:     `.a == 1`,
			useTransform: true,
		},
	}

	// Run the test cases
	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			marshaled, _ := json.Marshal(tc.input)
			// If `useTransform` is true, we first transform the input to JSON before running jq.Match
			// If `useTransform` is false, we directly pass the input to jq.Match.
			if tc.useTransform {
				g.Expect(tc.input).Should(WithTransform(json.Marshal, jq.Match(tc.jqQuery)))
			} else {
				g.Expect(tc.input).Should(jq.Match(tc.jqQuery))
			}
		})
	}
}

func TestUnstructuredSliceMatcher(t *testing.T) {
	t.Parallel()

	g := NewWithT(t)

	u := []unstructured.Unstructured{{
		Object: map[string]interface{}{
			"a": 1,
		}},
	}

	g.Expect(u).Should(
		jq.Match(`.[0] | .a == 1`))

	g.Expect(unstructured.UnstructuredList{Items: u}).Should(
		jq.Match(`.[0] | .a == 1`))
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can optimize it like this:

package jq

import (
	"encoding/json"
	"errors"
	"fmt"
	"io"
	"reflect"
	"strings"

	"github.com/onsi/gomega/format"
	"github.com/onsi/gomega/gbytes"
	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

func formattedMessage(comparisonMessage string, failurePath []interface{}) string {
	diffMessage := ""

	if len(failurePath) != 0 {
		diffMessage = "\n\nfirst mismatched key: " + formattedFailurePath(failurePath)
	}

	return comparisonMessage + diffMessage
}

func formattedFailurePath(failurePath []interface{}) string {
	formattedPaths := make([]string, 0)

	for i := len(failurePath) - 1; i >= 0; i-- {
		switch p := failurePath[i].(type) {
		case int:
			val := fmt.Sprintf(`[%d]`, p)
			formattedPaths = append(formattedPaths, val)
		default:
			if i != len(failurePath)-1 {
				formattedPaths = append(formattedPaths, ".")
			}

			val := fmt.Sprintf(`"%s"`, p)
			formattedPaths = append(formattedPaths, val)
		}
	}

	return strings.Join(formattedPaths, "")
}

//nolint:cyclop
func toType(in any) (any, error) {
	switch v := in.(type) {
	case string, []byte, json.RawMessage, *gbytes.Buffer:
		// Handle string, []byte, json.RawMessage, and *gbytes.Buffer by converting them into the appropriate type.
		return convertToJSON(v)
	case io.Reader:
		// Handle io.Reader by reading the content and converting it into the appropriate type.
		data, err := io.ReadAll(v)
		if err != nil {
			return nil, fmt.Errorf("failed to read from reader: %w", err)
		}
		return convertToJSON(data)
	case unstructured.UnstructuredList:
		// Handle UnstructuredList by extracting the objects from the list.
		return convertUnstructuredItemsToType(v.Items)
	case []unstructured.Unstructured:
		// Handle slice of Unstructured items.
		return convertUnstructuredItemsToType(v)
	case unstructured.Unstructured, *unstructured.Unstructured:
		// Handle single Unstructured item or pointer to Unstructured item.
		// Both types can be handled the same way by accessing the `Object` field.
		return v.(unstructured.Unstructured).Object, nil
	case []*unstructured.Unstructured:
		// Handle slice of pointers to Unstructured items.
		return convertUnstructuredItemsToType(v)
	}

	// Fallback case for maps and slices.
	switch reflect.TypeOf(in).Kind() {
	case reflect.Map:
		return in, nil
	case reflect.Slice:
		return in, nil
	default:
		return nil, fmt.Errorf("unsuported type:\n%s", format.Object(in, 1))
	}
}

// convertToJSON converts various types into a structured JSON object or slice.
func convertToJSON(v any) (any, error) {
	var data []byte
	switch value := v.(type) {
	case string:
		data = []byte(value)
	case []byte:
		data = value
	case json.RawMessage:
		data = []byte(value)
	case *gbytes.Buffer:
		data = value.Contents()
	}

	switch data[0] {
	case '{':
		var obj map[string]any
		if err := json.Unmarshal(data, &obj); err != nil {
			return nil, fmt.Errorf("unable to unmarshal JSON object, %w", err)
		}
		return obj, nil
	case '[':
		var arr []any
		if err := json.Unmarshal(data, &arr); err != nil {
			return nil, fmt.Errorf("unable to unmarshal JSON array, %w", err)
		}
		return arr, nil
	default:
		return nil, errors.New("JSON must be an array or object")
	}
}

// convertUnstructuredItemsToType converts Unstructured items into their map representation.
func convertUnstructuredItemsToType(items []unstructured.Unstructured) ([]any, error) {
	res := make([]any, 0, len(items))
	for i := range items {
		res = append(res, items[i].Object)
	}
	return res, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to send a pr :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

The Kustomize rendering engine embedded in the operator, does not
enforce any particular order, but relies on the order of the manifests
provided, however, Kustomize has options to change the strategy used
to sort resources at the end of the Kustomize build see:

- https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/sortoptions

This commit adds some test to ensure the sortOrder configuration is
honored by the operator's own Kustomize engine.
@openshift-ci openshift-ci bot added the lgtm label Feb 7, 2025
@MarianMacik
Copy link
Member

I will have a look shortly :)

@lburgazzoli
Copy link
Contributor Author

/hold

@lburgazzoli
Copy link
Contributor Author

/unhold

Copy link
Member

@MarianMacik MarianMacik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, nice!

Copy link

openshift-ci bot commented Feb 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MarianMacik, ugiordan, zdtsw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [MarianMacik,ugiordan,zdtsw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Feb 7, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 4b86e21 into opendatahub-io:main Feb 7, 2025
10 checks passed
@lburgazzoli lburgazzoli deleted the k-o branch February 8, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants