HIVE-3148: Ote hive#2894
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@miyadav: This pull request references HIVE-3148 which is a valid 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. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new OpenShift Hive OTE extension: Go module and build tooling, CLI entrypoint, embedded testdata helpers, many Ginkgo E2E suites (core, Azure, GCP, vSphere), numerous provider templates/fixtures, and Docker build changes to include the compiled test extension binary. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "test/ote CLI"
participant Registry as "Extension Registry"
participant Hive as "Hive API/Operator"
participant Cloud as "Cloud Provider"
participant Thanos as "Thanos / Monitoring"
CLI->>Registry: initialize registry & register Hive suite
CLI->>Registry: build extension test specs from Ginkgo suites
CLI->>Hive: create ClusterDeployment and related resources
Hive->>Cloud: request IaaS provisioning (VMs, networks)
Cloud-->>Hive: provisioned infra (spoke cluster)
Hive->>Thanos: expose metrics
CLI->>Thanos: query metrics for test assertions
CLI->>Hive: tear down created resources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
@miyadav: This pull request references HIVE-3148 which is a valid 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (6)
Dockerfile (1)
34-34: Prefer a single canonical extension binary name across Dockerfile andtest/ote/Makefile.This currently works, but it diverges from
test/ote/Makefileoutput (bin/hive) and can cause avoidable local-vs-image confusion. Consider standardizing on one target/name and reusing it in both places.Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` at line 34, The Dockerfile builds the extension binary as openshift-tests-extension which diverges from test/ote/Makefile's bin/hive; change the Dockerfile RUN that calls "cd test/ote && go build -o ..." to produce the canonical binary name used by the Makefile (e.g., bin/hive) and update any other Dockerfile occurrences (line noted around the second mention) to match; ensure test/ote/Makefile's build target and the Dockerfile use the same output path/name so local and image builds are identical.test/ote/Makefile (1)
12-14: Consider adding race detection and coverage for tests.The test target is functional, but enabling race detection and coverage reporting would improve test quality assurance.
♻️ Optional enhancement
.PHONY: test test: - go test ./... + go test -race -coverprofile=coverage.out ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/Makefile` around lines 12 - 14, The test Makefile target "test" currently runs "go test ./..." without race detection or coverage; update the "test" target to run tests with the race detector and generate a coverage report (e.g., use "go test -race -coverprofile=coverage.out ./..." and optionally "-covermode=atomic") and add or update a separate "coverage" target to run "go tool cover -html=coverage.out" so maintainers can view coverage; ensure the generated coverage file name and flags are consistent across targets and CI invocations.test/ote/hive/testdata/workload.yaml (1)
16-26: Consider adding a security context for the container.Static analysis tools flagged several security concerns. While this is test data for autoscaling validation, adding a minimal security context would align with best practices and prevent the test workload from running with elevated privileges.
🔒 Proposed security hardening
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - command: - sleep - "3600" image: quay.io/openshifttest/busybox@sha256:c5439d7db88ab5423999530349d327b04279ad3161d7596d2126dfb5b02bfd1f imagePullPolicy: Always name: sleep resources: requests: cpu: "1" + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/hive/testdata/workload.yaml` around lines 16 - 26, The container spec for the test workload (container name "sleep") lacks a securityContext; add a minimal securityContext under the container entry to enforce non-root and least-privilege settings (e.g., runAsNonRoot: true and runAsUser: 1000), disable privilege escalation (allowPrivilegeEscalation: false), drop capabilities (capabilities.drop: ["ALL"]), and enable a read-only root filesystem (readOnlyRootFilesystem: true); update the "sleep" container's spec accordingly so the test still runs but without elevated privileges.test/ote/cmd/extension/main.go (2)
28-30: Redundant error formatting - use%+vdirectly onerr.
err.Error()returns a string, so%+vjust prints the string. If the intent is to include a stack trace (for errors wrapped withpkg/errorsor similar), passerrdirectly to preserve the stack trace formatting.Suggested fix
if err != nil { - panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v", err.Error())) + panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v", err)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/cmd/extension/main.go` around lines 28 - 30, Replace the redundant err.Error() usage in the panic call with the error value itself so %+v can preserve any wrapped error details/stack; update the fmt.Sprintf call that currently does panic(fmt.Sprintf("couldn't build extension test specs from ginkgo: %+v", err.Error())) to pass err (the error variable) directly instead of err.Error().
41-43: Consider logging the error before exiting.When
root.Execute()fails, the error is discarded silently. Logging it would help with debugging CLI issues.Suggested fix
if err := root.Execute(); err != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/cmd/extension/main.go` around lines 41 - 43, When calling root.Execute() in main.go, capture the returned error and log it before exiting; replace the current if block (if err := root.Execute(); err != nil { os.Exit(1) }) with a version that assigns err, writes a descriptive message and the error to stderr (e.g., via fmt.Fprintln(os.Stderr, "root.Execute error:", err) or log.Printf) and then calls os.Exit(1), referencing root.Execute() and the err variable.test/ote/hive/testdata/gcp-install-config.yaml (1)
20-23: Consider using meaningful default values or parameterizing disk settings.
DiskSizeGB: 0andDiskType: ""are likely placeholders. If these values are passed to the installer, they may cause validation errors or unexpected behavior. Consider either parameterizing these or using sensible defaults (e.g.,diskSizeGB: 128,diskType: pd-ssd).Also applies to: 29-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/hive/testdata/gcp-install-config.yaml` around lines 20 - 23, The osDisk entries currently use placeholder values (DiskSizeGB: 0, DiskType: "") which can break installer validation; update the osDisk configuration in the GCP install YAML (keys: osDisk, DiskSizeGB, DiskType) to either parameterize these fields or set sensible defaults (for example DiskSizeGB: 128 and DiskType: pd-ssd) and ensure the machine type (type: n1-standard-4) remains compatible; apply the same change to the other occurrence mentioned (lines 29-32) so both blocks use valid/parameterized disk settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ote/bindata.mk`:
- Around line 10-21: The TESTDATA_DIR variable is relative to the current
working dir and makes the verify-testdata and list-testdata targets fail when
make is invoked from elsewhere; fix it by computing TESTDATA_DIR from the
Makefile location (use MAKEFILE_LIST to get the current Makefile dir and
abspath/dir to resolve hive/testdata) so TESTDATA_DIR becomes an absolute path
based on the Makefile, leaving the list-testdata and verify-testdata targets
unchanged except they should reference the new TESTDATA_DIR.
In `@test/ote/go.mod`:
- Line 3: The go directive in test/ote/go.mod is set to 1.25.0 which conflicts
with the CI/builder images pinned to Go 1.24; fix by either changing the go
directive in test/ote/go.mod from "go 1.25.0" to "go 1.24.0" to match the
existing Makefile/Dockerfile builder_rhel* image pins, or update the CI/build
image pins referenced in the Makefile and Dockerfile to use Go 1.25 so the
builder_rhel9 stage and CI use a compatible Go version.
In `@test/ote/hive/fixtures.go`:
- Around line 15-17: The FixturePath function currently joins from the package
directory and omits the testdata folder; update FixturePath to include
"testdata" in the path building (e.g., ensure the base path is
filepath.Dir(file) + "/testdata" or prepend "testdata" into the slice passed to
filepath.Join) so that FixturePath("foo.yaml") resolves to the package's
testdata/foo.yaml; update the join logic inside FixturePath to always insert
"testdata" between the package dir (from runtime.Caller) and the elem variadic
arguments.
In `@test/ote/hive/hive_azure.go`:
- Around line 427-434: The test currently parses the ClusterDeployment
condition's lastProbeTime into hibernateTimestamp and asserts difference from
claimedTimestamp, which is flaky; change the jsonpath to fetch
lastTransitionTime (replace
`-o=jsonpath={.status.conditions[?(@.type=="Hibernating")].lastProbeTime}` with
`.lastTransitionTime`), update the variable/log if desired (hibernateTimestamp
stays fine), keep parsing with time.Parse(time.RFC3339, ...) and assert
difference.Minutes() >= 5 as before using the new timestamp; ensure error
handling (o.Expect(err).NotTo(o.HaveOccurred())) remains.
In `@test/ote/hive/hive_gcp.go`:
- Around line 676-678: The generated clusterDeploymentRefs list currently emits
two separate YAML list items (one with only name and one with only namespace);
update the string/template that builds clusterDeploymentRefs in hive_gcp.go so
each list entry contains both fields together (e.g., a single list item with
both "name: "+cdName and "namespace: "+oc.Namespace() under the same dash),
ensuring the YAML indentation groups name and namespace into one map per list
element and preserving the existing apiGroup mutation for the negative test.
- Around line 137-140: The first GCP InstancesClient created with
compute.NewInstancesRESTClient(context.Background()) is never closed, leaking
transport resources; after creating instancesClient (the variable) add a defer
instancesClient.Close() immediately after the NewInstancesRESTClient call (or
explicitly call instancesClient.Close() after you finish using it around the
countVMs call) so the client is properly cleaned up; ensure the Close call uses
the same instancesClient variable referenced when calling countVMs and follows
the pattern used later in the file for the other InstancesClient.
In `@test/ote/hive/hive_vsphere.go`:
- Around line 73-78: The code assumes
infrastructure.Spec.PlatformSpec.VSphere.FailureDomains and each
FailureDomain.Topology.Networks are non-empty before indexing (used to set
datacenter, datastore, network, vCenter, cluster in BeforeEach); add explicit
guards that check len(infrastructure.Spec.PlatformSpec.VSphere.FailureDomains) >
0 and len(failureDomains[0].Topology.Networks) > 0 before accessing indices, and
if either check fails return/Fail/Skip the test with a clear error message
mentioning the missing FailureDomains or Networks so the suite fails with an
informative environment error instead of panicking.
In `@test/ote/hive/hive.go`:
- Around line 227-229: The Subscription check should not assume conditions[0]
order; update the jsonpath used in the newCheck invocation (the call building
the CLI args for "sub", sub.name, "-n", sub.namespace, ...) to select the
condition whose type equals "AllCatalogSourcesHealthy" instead of indexing [0];
e.g. replace "-o=jsonpath={.status.conditions[0].reason}" with a jsonpath that
filters by type (such as
"-o=jsonpath={.status.conditions[?(@.type==\"AllCatalogSourcesHealthy\")].reason}")
so the check reliably reads the correct condition reason.
In `@test/ote/hive/testdata/clusterdeployment-aws-assumerole.yaml`:
- Line 23: Replace the malformed OpenShift Template variables and fix the typed
field: change all occurrences of the double-braced syntax `${{MANAGEDNS}}` (and
any other `${{PARAMETER}}`) to the correct OpenShift Template form
`${MANAGEDNS}`, and ensure the manageDNS field is set as a boolean (unquoted) so
it emits true/false after substitution (e.g., manageDNS: ${MANAGEDNS}) instead
of a quoted string; update both occurrences referenced in the template
(manageDNS and the similar line at line 39) so validation against the CRD
succeeds.
In `@test/ote/hive/testdata/clusterdeployment-gcp.yaml`:
- Around line 20-49: The template is missing the SERVICEATTACHMENTSUBNETCIDR
parameter and wiring into spec.platform.gcp, so calls to
gcpClusterDeployment.create() that pass SERVICEATTACHMENTSUBNETCIDR silently
no-op when --ignore-unknown-parameters=true; add a parameter entry for
SERVICEATTACHMENTSUBNETCIDR in the parameters list and write it into the spec
under platform.gcp (e.g., spec.platform.gcp.serviceAttachmentSubnetCidr) so the
fixture accepts and renders the value, or alternatively remove the
SERVICEATTACHMENTSUBNETCIDR handling from gcpClusterDeployment.create() to make
the mismatch explicit.
In `@test/ote/hive/testdata/clusterpool-cdc.yaml`:
- Around line 18-24: The inventory block in the static YAML always emits three
ClusterDeploymentCustomization entries even when INVENTORYCDC1..3 are empty,
producing name: "" entries; update the template or helper so the inventory array
is only constructed from non-empty values: remove the static three-item list and
either (a) in the template render the inventory entries conditionally (only
include items when the corresponding INVENTORYCDC{1,2,3} variable is non-empty)
or (b) have clusterPoolCDC.create() assemble the inventory list dynamically and
inject it into the template so only present INVENTORYCDC names are added; ensure
you change references related to inventory and the clusterPoolCDC.create() call
accordingly.
In `@test/ote/hive/testdata/gcp-install-config-sharedvpc.yaml`:
- Line 48: The secureBoot field currently injects the literal template variable
SECUREBOOT which may be "true"/"false" but the GCP/OpenShift schema requires the
string values "Enabled" or "Disabled"; update the template so the secureBoot key
emits one of those strings (e.g., secureBoot: "Enabled" or secureBoot:
"Disabled") by mapping the SECUREBOOT input (or the code that sets SECUREBOOT)
to return "Enabled" when truthy and "Disabled" when falsy; ensure the template
uses the mapped value for the secureBoot field (reference the secureBoot YAML
key and the SECUREBOOT variable).
In `@test/ote/hive/testdata/gcp-install-config.yaml`:
- Around line 20-23: The osDisk properties in the install-config use incorrect
PascalCase names (DiskSizeGB, DiskType) which the OpenShift installer won't
recognize; update the osDisk mapping (the osDisk block containing DiskSizeGB and
DiskType) to use camelCase keys diskSizeGB and diskType (and make the same
change for the other osDisk instance noted around lines 29-32) so the installer
schema accepts these fields.
In `@test/ote/hive/testdata/hive-operator-podmonitor.yaml`:
- Around line 8-9: The PodMonitor manifest contains an empty bearerTokenSecret
entry ("bearerTokenSecret: key: \"\""); remove the entire bearerTokenSecret
section from hive-operator-podmonitor.yaml or replace it with a valid secret
reference (non-empty key and existing secret name) so the PodMonitor does not
reference an invalid/unused Kubernetes secret.
In `@test/ote/hive/testdata/machinepool-infra-aws.yaml`:
- Around line 23-26: The rootVolume configuration currently sets size: 22
(rootVolume.size) which is too small for infra nodes; update the
machinepool-infra template to use a larger, realistic root volume (e.g., size:
100 or a parameter like ${{INFRA_ROOT_SIZE}}) by changing rootVolume.size from
22 to the chosen value and optionally document or expose it as a variable to
keep parity with worker templates and allow test overrides.
- Around line 15-17: The YAML contains a malformed node-role label key
"node-role.kubernetes.io" with value "infra"; update the labels under "labels"
so the role is part of the key (replace or remove the incorrect
"node-role.kubernetes.io: infra" entry and ensure only
"node-role.kubernetes.io/infra: \"\"" remains) so the file uses the correct
Kubernetes node-role label format referenced by the keys
"node-role.kubernetes.io" and "node-role.kubernetes.io/infra".
In `@test/ote/hive/testdata/machinepool-worker-aws.yaml`:
- Around line 18-21: The rootVolume block sets size: 22 which is too small for
worker nodes; update the rootVolume.size value in the
machinepool-worker-aws.yaml test data to a more realistic value (e.g., 100 or
120) so worker nodes have sufficient disk (adjust any related test expectations
if they assert on size), keeping the existing keys rootVolume, iops and type
unchanged so only the size numeric value is modified.
---
Nitpick comments:
In `@Dockerfile`:
- Line 34: The Dockerfile builds the extension binary as
openshift-tests-extension which diverges from test/ote/Makefile's bin/hive;
change the Dockerfile RUN that calls "cd test/ote && go build -o ..." to produce
the canonical binary name used by the Makefile (e.g., bin/hive) and update any
other Dockerfile occurrences (line noted around the second mention) to match;
ensure test/ote/Makefile's build target and the Dockerfile use the same output
path/name so local and image builds are identical.
In `@test/ote/cmd/extension/main.go`:
- Around line 28-30: Replace the redundant err.Error() usage in the panic call
with the error value itself so %+v can preserve any wrapped error details/stack;
update the fmt.Sprintf call that currently does panic(fmt.Sprintf("couldn't
build extension test specs from ginkgo: %+v", err.Error())) to pass err (the
error variable) directly instead of err.Error().
- Around line 41-43: When calling root.Execute() in main.go, capture the
returned error and log it before exiting; replace the current if block (if err
:= root.Execute(); err != nil { os.Exit(1) }) with a version that assigns err,
writes a descriptive message and the error to stderr (e.g., via
fmt.Fprintln(os.Stderr, "root.Execute error:", err) or log.Printf) and then
calls os.Exit(1), referencing root.Execute() and the err variable.
In `@test/ote/hive/testdata/gcp-install-config.yaml`:
- Around line 20-23: The osDisk entries currently use placeholder values
(DiskSizeGB: 0, DiskType: "") which can break installer validation; update the
osDisk configuration in the GCP install YAML (keys: osDisk, DiskSizeGB,
DiskType) to either parameterize these fields or set sensible defaults (for
example DiskSizeGB: 128 and DiskType: pd-ssd) and ensure the machine type (type:
n1-standard-4) remains compatible; apply the same change to the other occurrence
mentioned (lines 29-32) so both blocks use valid/parameterized disk settings.
In `@test/ote/hive/testdata/workload.yaml`:
- Around line 16-26: The container spec for the test workload (container name
"sleep") lacks a securityContext; add a minimal securityContext under the
container entry to enforce non-root and least-privilege settings (e.g.,
runAsNonRoot: true and runAsUser: 1000), disable privilege escalation
(allowPrivilegeEscalation: false), drop capabilities (capabilities.drop:
["ALL"]), and enable a read-only root filesystem (readOnlyRootFilesystem: true);
update the "sleep" container's spec accordingly so the test still runs but
without elevated privileges.
In `@test/ote/Makefile`:
- Around line 12-14: The test Makefile target "test" currently runs "go test
./..." without race detection or coverage; update the "test" target to run tests
with the race detector and generate a coverage report (e.g., use "go test -race
-coverprofile=coverage.out ./..." and optionally "-covermode=atomic") and add or
update a separate "coverage" target to run "go tool cover -html=coverage.out" so
maintainers can view coverage; ensure the generated coverage file name and flags
are consistent across targets and CI invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 69819563-f570-486f-8db0-bf9a1da3384f
⛔ Files ignored due to path filters (1)
test/ote/go.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
.gitignoreDockerfiletest/ote/Makefiletest/ote/bindata.mktest/ote/cmd/extension/main.gotest/ote/go.modtest/ote/hive/fixtures.gotest/ote/hive/hive.gotest/ote/hive/hive_aws.gotest/ote/hive/hive_azure.gotest/ote/hive/hive_gcp.gotest/ote/hive/hive_util.gotest/ote/hive/hive_vsphere.gotest/ote/hive/testdata/aws-install-config-privatelink.yamltest/ote/hive/testdata/aws-install-config.yamltest/ote/hive/testdata/azure-install-config.yamltest/ote/hive/testdata/cloudformation-endpointvpc-temp.yamltest/ote/hive/testdata/cluster-monitoring-config.yamltest/ote/hive/testdata/clusterclaim.yamltest/ote/hive/testdata/clusterdeployment-adopt.yamltest/ote/hive/testdata/clusterdeployment-aws-assumerole.yamltest/ote/hive/testdata/clusterdeployment-aws-privatelink.yamltest/ote/hive/testdata/clusterdeployment-azure.yamltest/ote/hive/testdata/clusterdeployment-gcp-psc.yamltest/ote/hive/testdata/clusterdeployment-gcp.yamltest/ote/hive/testdata/clusterdeployment-vsphere.yamltest/ote/hive/testdata/clusterdeployment.yamltest/ote/hive/testdata/clusterdeploymentcustomizations.yamltest/ote/hive/testdata/clusterimageset.yamltest/ote/hive/testdata/clusterpool-azure.yamltest/ote/hive/testdata/clusterpool-cdc.yamltest/ote/hive/testdata/clusterpool-gcp.yamltest/ote/hive/testdata/clusterpool.yamltest/ote/hive/testdata/gcp-install-config-psc.yamltest/ote/hive/testdata/gcp-install-config-sharedvpc.yamltest/ote/hive/testdata/gcp-install-config.yamltest/ote/hive/testdata/hive-clustersync-servicemonitor.yamltest/ote/hive/testdata/hive-controllers-servicemonitor.yamltest/ote/hive/testdata/hive-operator-podmonitor.yamltest/ote/hive/testdata/hiveconfig.yamltest/ote/hive/testdata/linkvpc.pytest/ote/hive/testdata/machinepool-infra-aws.yamltest/ote/hive/testdata/machinepool-infra-azure.yamltest/ote/hive/testdata/machinepool-infra-gcp-sharedvpc.yamltest/ote/hive/testdata/machinepool-infra-gcp.yamltest/ote/hive/testdata/machinepool-worker-aws.yamltest/ote/hive/testdata/machinepool-worker-azure.yamltest/ote/hive/testdata/machinepool-worker-gcp.yamltest/ote/hive/testdata/machinepool-worker-vsphere.yamltest/ote/hive/testdata/namespace.yamltest/ote/hive/testdata/operatorgroup.yamltest/ote/hive/testdata/subscription.yamltest/ote/hive/testdata/syncset-patch.yamltest/ote/hive/testdata/syncset-resource.yamltest/ote/hive/testdata/syncset-secret.yamltest/ote/hive/testdata/vsphere-install-config.yamltest/ote/hive/testdata/workload.yaml
| TESTDATA_DIR := hive/testdata | ||
|
|
||
| .PHONY: list-testdata | ||
| list-testdata: | ||
| @find $(TESTDATA_DIR) -type f | sort | ||
|
|
||
| .PHONY: verify-testdata | ||
| verify-testdata: | ||
| @echo "Verifying testdata directory exists..." | ||
| @test -d $(TESTDATA_DIR) || (echo "ERROR: $(TESTDATA_DIR) not found" && exit 1) | ||
| @echo "Testdata directory OK: $(TESTDATA_DIR)" | ||
| @echo "Files: $$(find $(TESTDATA_DIR) -type f | wc -l)" |
There was a problem hiding this comment.
Make testdata path independent of invocation directory.
Line 10 currently depends on where make is run. If invoked from repo root, verify-testdata may check the wrong directory and fail even when fixtures exist under test/ote/hive/testdata.
Proposed fix
+MKFILE_DIR := $(dir $(abspath $(lastword $(MAKEFILE_LIST))))
-TESTDATA_DIR := hive/testdata
+TESTDATA_DIR := $(MKFILE_DIR)hive/testdata🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/bindata.mk` around lines 10 - 21, The TESTDATA_DIR variable is
relative to the current working dir and makes the verify-testdata and
list-testdata targets fail when make is invoked from elsewhere; fix it by
computing TESTDATA_DIR from the Makefile location (use MAKEFILE_LIST to get the
current Makefile dir and abspath/dir to resolve hive/testdata) so TESTDATA_DIR
becomes an absolute path based on the Makefile, leaving the list-testdata and
verify-testdata targets unchanged except they should reference the new
TESTDATA_DIR.
| func FixturePath(elem ...string) string { | ||
| _, file, _, _ := runtime.Caller(0) | ||
| return filepath.Join(append([]string{filepath.Dir(file)}, elem...)...) |
There was a problem hiding this comment.
FixturePath does not include the testdata directory as documented.
Line 17 currently joins from the package directory, so FixturePath("foo.yaml") resolves to .../hive/foo.yaml instead of .../hive/testdata/foo.yaml.
Suggested fix
func FixturePath(elem ...string) string {
_, file, _, _ := runtime.Caller(0)
- return filepath.Join(append([]string{filepath.Dir(file)}, elem...)...)
+ parts := append([]string{filepath.Dir(file), "testdata"}, elem...)
+ return filepath.Join(parts...)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func FixturePath(elem ...string) string { | |
| _, file, _, _ := runtime.Caller(0) | |
| return filepath.Join(append([]string{filepath.Dir(file)}, elem...)...) | |
| func FixturePath(elem ...string) string { | |
| _, file, _, _ := runtime.Caller(0) | |
| parts := append([]string{filepath.Dir(file), "testdata"}, elem...) | |
| return filepath.Join(parts...) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/hive/fixtures.go` around lines 15 - 17, The FixturePath function
currently joins from the package directory and omits the testdata folder; update
FixturePath to include "testdata" in the path building (e.g., ensure the base
path is filepath.Dir(file) + "/testdata" or prepend "testdata" into the slice
passed to filepath.Join) so that FixturePath("foo.yaml") resolves to the
package's testdata/foo.yaml; update the join logic inside FixturePath to always
insert "testdata" between the package dir (from runtime.Caller) and the elem
variadic arguments.
| - bearerTokenSecret: | ||
| key: "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other PodMonitors/ServiceMonitors in the codebase use similar empty bearerTokenSecret patterns
rg -n "bearerTokenSecret" --type yaml -A 2 -B 1Repository: openshift/hive
Length of output: 356
🏁 Script executed:
cat -n test/ote/hive/testdata/hive-operator-podmonitor.yamlRepository: openshift/hive
Length of output: 649
🏁 Script executed:
# Check for any related documentation or comments in the test directory
fd -t f -e md -e txt test/ote/hive/testdata/ | head -20Repository: openshift/hive
Length of output: 455
🏁 Script executed:
# Search for other PodMonitor configurations or examples that might show expected patterns
find test -name "*podmonitor*" -o -name "*prometheus*" -o -name "*monitoring*" | head -20Repository: openshift/hive
Length of output: 164
🏁 Script executed:
cat -n test/ote/hive/testdata/cluster-monitoring-config.yamlRepository: openshift/hive
Length of output: 524
🏁 Script executed:
# Search for any git history or related issues about this PodMonitor
rg "hive-operator-podmonitor" test/ --type yaml -B 3 -A 3Repository: openshift/hive
Length of output: 40
🏁 Script executed:
# Check if there are any Kubernetes/Prometheus examples in the codebase showing proper bearerTokenSecret usage
rg "bearerTokenSecret" . --type yaml -B 2 -A 2 2>/dev/null || echo "No other occurrences"Repository: openshift/hive
Length of output: 416
Remove the unused bearerTokenSecret field or provide a valid secret reference.
The bearerTokenSecret has an empty key value, which is not a valid Kubernetes secret reference. Since the metrics endpoint uses HTTP (not HTTPS) and no authentication is configured elsewhere, the bearerTokenSecret field should be removed entirely for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/hive/testdata/hive-operator-podmonitor.yaml` around lines 8 - 9, The
PodMonitor manifest contains an empty bearerTokenSecret entry
("bearerTokenSecret: key: \"\""); remove the entire bearerTokenSecret section
from hive-operator-podmonitor.yaml or replace it with a valid secret reference
(non-empty key and existing secret name) so the PodMonitor does not reference an
invalid/unused Kubernetes secret.
| labels: | ||
| node-role.kubernetes.io: infra | ||
| node-role.kubernetes.io/infra: "" |
There was a problem hiding this comment.
Malformed node-role label on line 16.
The label node-role.kubernetes.io: infra doesn't follow the Kubernetes node-role convention. Node role labels use the format node-role.kubernetes.io/<role>: "" where the role is part of the key, not the value. Line 17 is correct; line 16 appears to be incorrect and should likely be removed or fixed.
Suggested fix - remove the malformed label
labels:
- node-role.kubernetes.io: infra
node-role.kubernetes.io/infra: ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| labels: | |
| node-role.kubernetes.io: infra | |
| node-role.kubernetes.io/infra: "" | |
| labels: | |
| node-role.kubernetes.io/infra: "" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/hive/testdata/machinepool-infra-aws.yaml` around lines 15 - 17, The
YAML contains a malformed node-role label key "node-role.kubernetes.io" with
value "infra"; update the labels under "labels" so the role is part of the key
(replace or remove the incorrect "node-role.kubernetes.io: infra" entry and
ensure only "node-role.kubernetes.io/infra: \"\"" remains) so the file uses the
correct Kubernetes node-role label format referenced by the keys
"node-role.kubernetes.io" and "node-role.kubernetes.io/infra".
| rootVolume: | ||
| iops: ${{IOPS}} | ||
| size: 22 | ||
| type: gp2 |
There was a problem hiding this comment.
Same small root volume size concern as worker template.
22GB root volume is small for infra nodes, which run infrastructure workloads like routers and monitoring. Consider aligning with typical production values (100GB+) for realistic test coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/hive/testdata/machinepool-infra-aws.yaml` around lines 23 - 26, The
rootVolume configuration currently sets size: 22 (rootVolume.size) which is too
small for infra nodes; update the machinepool-infra template to use a larger,
realistic root volume (e.g., size: 100 or a parameter like ${{INFRA_ROOT_SIZE}})
by changing rootVolume.size from 22 to the chosen value and optionally document
or expose it as a variable to keep parity with worker templates and allow test
overrides.
| rootVolume: | ||
| iops: ${{IOPS}} | ||
| size: 22 | ||
| type: gp2 |
There was a problem hiding this comment.
Root volume size of 22GB may be too small for worker nodes.
A 22GB root volume is unusually small for OpenShift worker nodes, which typically require 100-120GB to accommodate container images and runtime data. While this is test data, ensure tests won't fail due to disk pressure if real clusters are provisioned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/hive/testdata/machinepool-worker-aws.yaml` around lines 18 - 21, The
rootVolume block sets size: 22 which is too small for worker nodes; update the
rootVolume.size value in the machinepool-worker-aws.yaml test data to a more
realistic value (e.g., 100 or 120) so worker nodes have sufficient disk (adjust
any related test expectations if they assert on size), keeping the existing keys
rootVolume, iops and type unchanged so only the size numeric value is modified.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2894 +/- ##
==========================================
+ Coverage 50.28% 50.49% +0.20%
==========================================
Files 280 280
Lines 34332 34313 -19
==========================================
+ Hits 17264 17326 +62
+ Misses 15707 15630 -77
+ Partials 1361 1357 -4 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/ote/hive/hive_gcp.go (2)
678-680: Consider adding explicitnamespacetoclusterDeploymentRefsfor clarity.The
clusterDeploymentRefsentry only specifiesname. While namespace defaults to the SyncSet's namespace when omitted, adding it explicitly would improve readability and make the reference unambiguous:spec: clusterDeploymentRefs: - name: ` + cdName + ` + namespace: ` + oc.Namespace() + ` resourceApplyMode: Sync🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/hive/hive_gcp.go` around lines 678 - 680, clusterDeploymentRefs only includes name which leaves the namespace implicit; update the SyncSet/YAML snippet where clusterDeploymentRefs is constructed (the block containing clusterDeploymentRefs and the cdName variable) to add an explicit namespace field matching the SyncSet's namespace (e.g., namespace: <syncSetNamespace>), so the reference is unambiguous and readable while keeping resourceApplyMode: Sync intact.
6-6: Replace deprecatedioutilpackage withos.
ioutil.WriteFileandioutil.ReadFilewere deprecated in Go 1.16. Useos.WriteFileinstead.Proposed fix
import ( "context" "fmt" - "io/ioutil" "os" "path/filepath"And update usages at lines 694 and 943:
- err = ioutil.WriteFile(filename, []byte(syncsetYaml), 0644) + err = os.WriteFile(filename, []byte(syncsetYaml), 0644)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/hive/hive_gcp.go` at line 6, Replace the deprecated ioutil import and calls: in hive_gcp.go remove the "io/ioutil" import and add "os" instead, then change all usages of ioutil.WriteFile and ioutil.ReadFile (referenced around the current usages) to os.WriteFile and os.ReadFile respectively so the file uses the modern os package APIs.test/ote/hive/hive.go (1)
281-284:conditions[0]index may be fragile for HiveConfig status checks.Reading
conditions[0].typeandconditions[0].reasonassumes stable ordering. While this is used to fetch current values for comparison against metrics (not asserting specific values), if condition order changes between reads, the test could get inconsistent data.Consider using a filter if you need a specific condition type, or documenting that the test intentionally reads "whatever condition is first" for metric comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/hive/hive.go` around lines 281 - 284, The current reads of conditions[0] via expectedType/expectedReason are fragile because condition ordering can change; update the oc.AsAdmin().WithoutNamespace().Run("get").Args(... "-o=jsonpath=...") calls used to populate expectedType and expectedReason to select the specific condition by its .type (e.g., use a jsonpath that filters .status.conditions by condition.type or extracts the condition whose .type == "<desiredType>") instead of hardcoding index [0], or alternatively add a clear comment next to the expectedType/expectedReason assignments documenting that the test intentionally reads the first condition for metric comparison and that ordering instability is acceptable for this check. Ensure you modify the jsonpath string used in the oc...Run("get") call or add the comment near the expectedType/expectedReason variables.test/ote/hive/fixtures.go (1)
34-60: Consider cleaning up the temporary extraction directory.The extracted testdata directory (
extractedDir) is never cleaned up. While this is acceptable for short-lived test processes, it could leave orphaned temp directories if tests are interrupted. Consider registering a cleanup function or documenting this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ote/hive/fixtures.go` around lines 34 - 60, The temp extraction created by extractOnce (setting extractedDir) is never removed; add a public cleanup function (e.g., CleanupExtractedDir) that checks extractedDir != "" and calls os.RemoveAll(extractedDir) (and clears extractedDir) so tests can remove the temp dir, and document or call that from TestMain or individual tests' t.Cleanup; reference the extractOnce variable and extractedDir and the testdataFS extraction logic so the cleanup runs after use and is safe to call multiple times.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/ote/hive/hive_gcp.go`:
- Line 52: installHiveOperator's two return values are being discarded which can
hide installation failures; assign them to variables (e.g., result, err :=
installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir)), then check the error
(if err != nil) and handle it appropriately in this test (log and fail the test
with t.Fatalf or return the error) so setup failures are surfaced; update all
occurrences (including the similar case in hive.go) that currently use "_, _ ="
to capture and check the error.
In `@test/ote/hive/hive.go`:
- Line 41: installHiveOperator's returned status and error are being ignored;
capture both (e.g., status, err := installHiveOperator(oc, &ns, &og, &sub, &hc,
testDataDir)), check err and fail the test immediately on error (use t.Fatalf or
similar) and also validate the returned status string (fail if it does not
indicate success) so setup failures surface clearly; update the call site in
hive.go to handle these two return values and provide a clear failure message
including the error and status.
---
Nitpick comments:
In `@test/ote/hive/fixtures.go`:
- Around line 34-60: The temp extraction created by extractOnce (setting
extractedDir) is never removed; add a public cleanup function (e.g.,
CleanupExtractedDir) that checks extractedDir != "" and calls
os.RemoveAll(extractedDir) (and clears extractedDir) so tests can remove the
temp dir, and document or call that from TestMain or individual tests'
t.Cleanup; reference the extractOnce variable and extractedDir and the
testdataFS extraction logic so the cleanup runs after use and is safe to call
multiple times.
In `@test/ote/hive/hive_gcp.go`:
- Around line 678-680: clusterDeploymentRefs only includes name which leaves the
namespace implicit; update the SyncSet/YAML snippet where clusterDeploymentRefs
is constructed (the block containing clusterDeploymentRefs and the cdName
variable) to add an explicit namespace field matching the SyncSet's namespace
(e.g., namespace: <syncSetNamespace>), so the reference is unambiguous and
readable while keeping resourceApplyMode: Sync intact.
- Line 6: Replace the deprecated ioutil import and calls: in hive_gcp.go remove
the "io/ioutil" import and add "os" instead, then change all usages of
ioutil.WriteFile and ioutil.ReadFile (referenced around the current usages) to
os.WriteFile and os.ReadFile respectively so the file uses the modern os package
APIs.
In `@test/ote/hive/hive.go`:
- Around line 281-284: The current reads of conditions[0] via
expectedType/expectedReason are fragile because condition ordering can change;
update the oc.AsAdmin().WithoutNamespace().Run("get").Args(...
"-o=jsonpath=...") calls used to populate expectedType and expectedReason to
select the specific condition by its .type (e.g., use a jsonpath that filters
.status.conditions by condition.type or extracts the condition whose .type ==
"<desiredType>") instead of hardcoding index [0], or alternatively add a clear
comment next to the expectedType/expectedReason assignments documenting that the
test intentionally reads the first condition for metric comparison and that
ordering instability is acceptable for this check. Ensure you modify the
jsonpath string used in the oc...Run("get") call or add the comment near the
expectedType/expectedReason variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3160e51a-ff2b-4c50-9df2-fe52cdb9052c
📒 Files selected for processing (10)
Dockerfiletest/ote/go.modtest/ote/hive/fixtures.gotest/ote/hive/hive.gotest/ote/hive/hive_azure.gotest/ote/hive/hive_gcp.gotest/ote/hive/hive_vsphere.gotest/ote/hive/testdata/gcp-install-config-psc.yamltest/ote/hive/testdata/gcp-install-config-sharedvpc.yamltest/ote/hive/testdata/gcp-install-config.yaml
✅ Files skipped from review due to trivial changes (5)
- test/ote/hive/testdata/gcp-install-config-psc.yaml
- test/ote/hive/testdata/gcp-install-config.yaml
- test/ote/hive/hive_vsphere.go
- test/ote/hive/testdata/gcp-install-config-sharedvpc.yaml
- test/ote/hive/hive_azure.go
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
|
|
||
| // Install Hive operator if not | ||
| testDataDir = FixturePath("testdata") | ||
| _, _ = installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir) |
There was a problem hiding this comment.
Ignoring installHiveOperator return values may hide setup failures.
Same issue as in hive.go - both return values are discarded, potentially hiding operator installation failures.
Proposed fix
- _, _ = installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir)
+ status, err := installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir)
+ if err != nil {
+ g.Skip("Failed to install Hive operator: " + err.Error())
+ }
+ e2e.Logf("Hive operator installation status: %s", status)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, _ = installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir) | |
| status, err := installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir) | |
| if err != nil { | |
| g.Skip("Failed to install Hive operator: " + err.Error()) | |
| } | |
| e2e.Logf("Hive operator installation status: %s", status) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/hive/hive_gcp.go` at line 52, installHiveOperator's two return
values are being discarded which can hide installation failures; assign them to
variables (e.g., result, err := installHiveOperator(oc, &ns, &og, &sub, &hc,
testDataDir)), then check the error (if err != nil) and handle it appropriately
in this test (log and fail the test with t.Fatalf or return the error) so setup
failures are surfaced; update all occurrences (including the similar case in
hive.go) that currently use "_, _ =" to capture and check the error.
|
|
||
| //Install Hive operator if not | ||
| testDataDir = FixturePath("testdata") | ||
| _, _ = installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir) |
There was a problem hiding this comment.
Ignoring installHiveOperator return values may hide setup failures.
Both the status string and error from installHiveOperator are discarded. If the operator installation fails, the tests will proceed and fail with confusing errors rather than a clear setup failure message.
Proposed fix
- _, _ = installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir)
+ status, err := installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir)
+ if err != nil {
+ g.Skip("Failed to install Hive operator: " + err.Error())
+ }
+ e2e.Logf("Hive operator installation status: %s", status)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ote/hive/hive.go` at line 41, installHiveOperator's returned status and
error are being ignored; capture both (e.g., status, err :=
installHiveOperator(oc, &ns, &og, &sub, &hc, testDataDir)), check err and fail
the test immediately on error (use t.Fatalf or similar) and also validate the
returned status string (fail if it does not indicate success) so setup failures
surface clearly; update the call site in hive.go to handle these two return
values and provide a clear failure message including the error and status.
|
I'm not likely to read 16KLOC of generated test code. However, I'll note that we probably don't want to add OTE to our production Dockerfile. Let's explore other solutions. Perhaps we have a separate Dockerfile.ote, and create a separate |
|
/test e2e-vsphere Trying to flush out this failure. TL;DR: we want to see this succeed with |
got you , thanks !! |
…t a payload binary
Sorry, I should have left more details. This test attempt was not related -- I was just hijacking your PR to run it. It did indeed succeed, but it was using /test e2e-vsphere |
Thanks Eric for review, I validated this locally for pushing image and binary working as per requirement of OTE , if we can merge this , then merge - openshift/origin#31059 as it is just registering the binary we create here and then use /pj-rehearse on openshift/release#78242 to confirm everything is in place and we are good to add tests in this repo then ( there might be few misses here and there as I understand and those we will fix along the way is what I am thinking ) will involve trt team as well if someone can review before the merge that we are going in the right direction. |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
Ideally we would (also?) be able to run them on demand...
...but running as periodics requires them to be defined as periodics. So we may end up needing both defs. I guess we can sort this out in the release PR. Since the changes here are not in production code paths, I'm happy to land it so we can get the other pieces in place and iterate as needed. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 2uasimojo, miyadav 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 |
|
@2uasimojo: Overrode contexts on behalf of 2uasimojo: ci/prow/security 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. |
openshift-ci
Bot
added
the
approved
|
@miyadav: all tests passed! 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. |
3 participants
/hold
PTAL if time permits , tested locally , but need to figure out a way to test as CI , not too sure about that , will be looking how to do that .
Used ai-helper ote:migrate , still work in progress.
Summary by CodeRabbit