NO-JIRA: Common provider manifest fixtures#498
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mdbooth: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughRemoved ad-hoc filesystem-based test manifest helpers and updated tests to use a new test fixtures builder in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/test/provider_fixtures.go (1)
141-144: Map iteration order is non-deterministic for multi-key data maps.When
ConfigMapYAMLis called with a custom multi-key data map, the YAML output order varies between runs due to Go's randomized map iteration. This could cause flaky tests if the output is used in content hashing. Current usage appears safe (single-key defaults), but consider sorting keys if multi-key maps become common.♻️ Optional fix for deterministic key ordering
func ConfigMapYAML(name string, data ...map[string]string) string { d := map[string]string{"key": "value"} if len(data) > 0 { d = data[0] } + keys := make([]string, 0, len(d)) + for k := range d { + keys = append(keys, k) + } + sort.Strings(keys) + var pairs strings.Builder - for k, v := range d { + for _, k := range keys { + v := d[k] fmt.Fprintf(&pairs, " %s: %s\n", k, v) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/provider_fixtures.go` around lines 141 - 144, The code in ConfigMapYAML builds YAML by iterating map d into the strings.Builder pairs, but Go map iteration is non-deterministic; to make output stable for multi-key maps, collect the keys from d, sort them (e.g., sort.Strings), and then iterate over the sorted key slice to fmt.Fprintf into pairs instead of ranging directly over d; update uses of pairs/ConfigMapYAML accordingly to ensure deterministic ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/test/provider_fixtures.go`:
- Around line 141-144: The code in ConfigMapYAML builds YAML by iterating map d
into the strings.Builder pairs, but Go map iteration is non-deterministic; to
make output stable for multi-key maps, collect the keys from d, sort them (e.g.,
sort.Strings), and then iterate over the sorted key slice to fmt.Fprintf into
pairs instead of ranging directly over d; update uses of pairs/ConfigMapYAML
accordingly to ensure deterministic ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 61a01c55-21c4-4918-92c4-bb5b5671e5bf
📒 Files selected for processing (6)
pkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/revisiongenerator/helpers_test.gopkg/revisiongenerator/revision_test.gopkg/test/provider_fixtures.go
💤 Files with no reviewable changes (1)
- pkg/controllers/revision/helpers_test.go
| // Each provider uses its ContentID as the ConfigMap name so that | ||
| // different provider sets produce different revision contentIDs. | ||
| defaultProviderImgs = []providerimages.ProviderImageManifests{ | ||
| test.NewTestProvider(tb, "core", |
There was a problem hiding this comment.
NewTestProvider creates what? What is a provider? Looks like this should actually be NewTestProviderImageManifests?
| ) | ||
|
|
||
| // ProviderOption configures a test provider created by NewTestProvider. | ||
| type ProviderOption func(*providerConfig, string) |
There was a problem hiding this comment.
Didn't I see another PR making similar builder patterns chain style rather than option style? Maybe be consistent?
NewTestProvider().WithImageRef(...)...
| // CRDAsYAML marshals a CRD object to YAML. Use with GenerateCRD or | ||
| // GenerateSchemalessSpecStatusCRD from crdbuilder.go to create CRD fixtures | ||
| // for use with WithManifests. | ||
| func CRDAsYAML(crd *apiextensionsv1.CustomResourceDefinition) string { |
There was a problem hiding this comment.
CRDToYaml? It took me a moment to think what a CRDA was and why you were creating a collection of them represented in YAML
mdbooth
force-pushed
the
provider-test-fixtures
branch
from
fd72584 to
d28b6d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/test/provider_fixtures.go (1)
91-96: Copy attribute maps to avoid shared mutable test state.
WithAttributesstores the caller map by reference on first set, andBuildreturns the same map reference. Later mutations can leak across fixtures.Proposed fix
func (b *ProviderImageManifestsBuilder) WithAttributes(attrs map[string]string) *ProviderImageManifestsBuilder { if b.attributes == nil { - b.attributes = attrs + b.attributes = maps.Clone(attrs) } else { maps.Copy(b.attributes, attrs) } @@ p := providerimages.ProviderImageManifests{ ProviderMetadata: providerimages.ProviderMetadata{ Name: b.name, InstallOrder: b.installOrder, OCPPlatform: b.platform, - Attributes: b.attributes, + Attributes: maps.Clone(b.attributes), SelfImageRef: b.selfImageRef, },As per coding guidelines,
**: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.Also applies to: 117-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/provider_fixtures.go` around lines 91 - 96, The WithAttributes method on ProviderImageManifestsBuilder currently stores the caller's map by reference which allows shared mutable state to leak; change WithAttributes to copy the incoming attrs into a new map (e.g., allocate a new map and copy entries) instead of assigning attrs directly, and also ensure the Build method of ProviderImageManifestsBuilder returns a copy of the attributes map (not the internal map pointer) so callers cannot mutate fixture internals; update any similar helper (the other With* method at the same area) the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/test/provider_fixtures.go`:
- Around line 19-25: ConfigMapYAML is producing nondeterministic YAML because it
iterates a Go map directly; update the function to collect the map's keys, sort
them (e.g., using slices.Sort or sort.Strings), and iterate the sorted keys to
build the YAML string so output is deterministic; apply the same sorted-key
approach to the other map-to-YAML helper(s) referenced near the same area to
ensure fixture output is stable across runs.
- Around line 49-58: NewProviderImageManifests can hang because SHA256Pad never
progresses for an empty name; add an explicit guard at the start of
NewProviderImageManifests that checks if name == "" and call tb.Fatalf("provider
name must not be empty") (or tb.Fatal) to fail fast, and apply the same guard to
any other constructor that builds imageRef via SHA256Pad (i.e., any function
that uses SHA256Pad to generate the imageRef) so tests never enter the infinite
loop.
---
Nitpick comments:
In `@pkg/test/provider_fixtures.go`:
- Around line 91-96: The WithAttributes method on ProviderImageManifestsBuilder
currently stores the caller's map by reference which allows shared mutable state
to leak; change WithAttributes to copy the incoming attrs into a new map (e.g.,
allocate a new map and copy entries) instead of assigning attrs directly, and
also ensure the Build method of ProviderImageManifestsBuilder returns a copy of
the attributes map (not the internal map pointer) so callers cannot mutate
fixture internals; update any similar helper (the other With* method at the same
area) the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4396178a-0fd1-4e32-afd3-6ac3ca7739c4
📒 Files selected for processing (6)
pkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/revisiongenerator/helpers_test.gopkg/revisiongenerator/revision_test.gopkg/test/provider_fixtures.go
💤 Files with no reviewable changes (1)
- pkg/controllers/revision/helpers_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/controllers/revision/suite_test.go
- pkg/revisiongenerator/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/revisiongenerator/revision_test.go
| import ( | ||
| "fmt" | ||
| "maps" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" |
There was a problem hiding this comment.
Make ConfigMapYAML deterministic by sorting map keys.
Iterating a Go map produces random order, so fixture output can vary between runs and cause flaky assertions.
Proposed fix
import (
"fmt"
"maps"
"os"
"path/filepath"
+ "sort"
"strings"
"testing"
@@
var pairs strings.Builder
- for k, v := range d {
+ keys := make([]string, 0, len(d))
+ for k := range d {
+ keys = append(keys, k)
+ }
+ sort.Strings(keys)
+ for _, k := range keys {
+ v := d[k]
fmt.Fprintf(&pairs, " %s: %s\n", k, v)
}As per coding guidelines, **: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Also applies to: 154-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/test/provider_fixtures.go` around lines 19 - 25, ConfigMapYAML is
producing nondeterministic YAML because it iterates a Go map directly; update
the function to collect the map's keys, sort them (e.g., using slices.Sort or
sort.Strings), and iterate the sorted keys to build the YAML string so output is
deterministic; apply the same sorted-key approach to the other map-to-YAML
helper(s) referenced near the same area to ensure fixture output is stable
across runs.
There was a problem hiding this comment.
@mdbooth we're not using this at the mo, but might be worth doing?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
mdbooth
force-pushed
the
provider-test-fixtures
branch
from
d28b6d3 to
8bfdfda
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/test/provider_fixtures.go (2)
154-157:⚠️ Potential issue | 🟠 MajorMap iteration produces non-deterministic YAML output.
Go maps iterate in random order, so the generated ConfigMap data keys may appear in different order across runs. This can cause flaky test assertions when comparing YAML strings.
Proposed fix using sorted keys
+ "slices" + "sort" "strings" @@ var pairs strings.Builder + keys := make([]string, 0, len(d)) for k, v := range d { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + v := d[k] fmt.Fprintf(&pairs, " %s: %s\n", k, v) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/provider_fixtures.go` around lines 154 - 157, The loop that writes map entries into the strings.Builder "pairs" iterates over map "d" which is nondeterministic; to fix, collect the keys from "d" into a slice, sort them (e.g., sort.Strings(keys)), then iterate over the sorted keys and append the entries to "pairs" (using fmt.Fprintf or pairs.WriteString) so generated YAML order is stable; update the loop that currently reads "for k, v := range d { fmt.Fprintf(&pairs, ...)" to the sorted-key approach.
220-232:⚠️ Potential issue | 🔴 CriticalEmpty
namecauses an infinite loop inSHA256Pad.When
name == "", the loop conditionlen(padded) < 64is always true becausepadded += namenever increases the string length. This will hang the test process.Proposed fix
func SHA256Pad(name string) string { + if name == "" { + return strings.Repeat("0", 64) + } + padded := name for len(padded) < 64 { padded += name }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/test/provider_fixtures.go` around lines 220 - 232, SHA256Pad currently infinite-loops when name == "" because padded += name never grows; add a guard at the start of SHA256Pad to handle empty input (e.g., if name == "" return strings.Repeat("0", 64)) so the function returns a valid 64-char hex string instead of looping; update the SHA256Pad function to short-circuit on empty name before constructing padded and the hex builder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/test/provider_fixtures.go`:
- Around line 154-157: The loop that writes map entries into the strings.Builder
"pairs" iterates over map "d" which is nondeterministic; to fix, collect the
keys from "d" into a slice, sort them (e.g., sort.Strings(keys)), then iterate
over the sorted keys and append the entries to "pairs" (using fmt.Fprintf or
pairs.WriteString) so generated YAML order is stable; update the loop that
currently reads "for k, v := range d { fmt.Fprintf(&pairs, ...)" to the
sorted-key approach.
- Around line 220-232: SHA256Pad currently infinite-loops when name == ""
because padded += name never grows; add a guard at the start of SHA256Pad to
handle empty input (e.g., if name == "" return strings.Repeat("0", 64)) so the
function returns a valid 64-char hex string instead of looping; update the
SHA256Pad function to short-circuit on empty name before constructing padded and
the hex builder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 434afc1c-3583-4d1f-a8f5-692c2ea49814
📒 Files selected for processing (6)
pkg/controllers/revision/helpers_test.gopkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/revisiongenerator/helpers_test.gopkg/revisiongenerator/revision_test.gopkg/test/provider_fixtures.go
💤 Files with no reviewable changes (1)
- pkg/controllers/revision/helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/revisiongenerator/revision_test.go
- pkg/revisiongenerator/helpers_test.go
- pkg/controllers/revision/revision_controller_test.go
mdbooth
force-pushed
the
provider-test-fixtures
branch
from
8bfdfda to
ef61031
Compare
|
one nit / agreement with coderabbit /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: theobarberbany The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Scheduling tests matching the |
openshift-ci
Bot
added
the
approved
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@mdbooth: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/pipeline auto |
|
Pipeline controller notification The |
|
/test regression-clusterinfra-aws-ipi-techpreview-capi |
|
/test e2e-aws-ovn-serial-1of2 |
|
This is wrong, but it'll tell me the right string: |
|
@mdbooth: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Ignoring permafailing job /override pull-ci-openshift-cluster-capi-operator-main-e2e-openstack-ovn-techpreview |
|
@mdbooth: Overrode contexts on behalf of mdbooth: ci/prow/e2e-openstack-ovn-techpreview DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@mdbooth: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-merge-bot
Bot
merged commit 49634c3
into
openshift:main
4 participants
Adds some common test fixtures for various packages which use them.