NO-JIRA: Fix 2 failures in ci jobs#485
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
sunzhaohua2
changed the title
📝 WalkthroughWalkthroughChanged CAPI machine selection to explicitly target worker-labeled machines, added a guard to skip tests when no worker CAPI machines exist, set bootstrap.DataSecretName for created CAPI machines, and clear specific MAPI spec.metadata.labels to prevent MachineSet adoption. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci-robot
added
the
jira/valid-reference
|
@sunzhaohua2: 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. |
|
/retest |
1 similar comment
|
/retest |
|
Aha! Nice to see this getting fixed :) Was the additional event data added on failure useful? e.g: /retest |
Yes! Super useful, it makes debug easier. Thanks you! |
|
/retest |
|
/test images |
|
/test ci/prow/e2e-aws-capi-techpreview |
|
/test e2e-aws-capi-techpreview |
e2e/machine_migration_helpers.go
Outdated
| // Skip test if no worker CAPI machines exist to use as reference | ||
| if len(capiMachineList) == 0 { | ||
| Skip("No worker CAPI machines found in cluster. This test requires at least one existing worker CAPI machine as a reference.") | ||
| } |
There was a problem hiding this comment.
This createCAPIMachine is a helper right? Not sure it makes sense to skip a whole test independently of what it is, based on the fact that it uses this helper and the helper is unable to create a Machine because one does not exist already, right?
There was a problem hiding this comment.
Thanks @damdo I have updated the pr, PTAL when you have a moment!
sunzhaohua2
force-pushed
the
fix-secret-issue
branch
from
ff17838 to
69fbffb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/machine_migration_helpers.go (1)
92-92: Consider extracting magic string to a named constant.The hardcoded
"worker-user-data"secret name fixes the CI failure as intended. For maintainability, consider defining this as a package-level constant, making it easier to update if the secret name changes and improving discoverability.Suggested constant definition
const workerBootstrapDataSecretName = "worker-user-data"Then use
ptr.To(workerBootstrapDataSecretName)on line 92.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/machine_migration_helpers.go` at line 92, Replace the hardcoded "worker-user-data" string with a package-level constant (e.g., define const workerBootstrapDataSecretName = "worker-user-data") and update the assignment to newCapiMachine.Spec.Bootstrap.DataSecretName to use ptr.To(workerBootstrapDataSecretName); this centralizes the magic string for easier maintenance and discovery while keeping the existing assignment in the newCapiMachine.Spec.Bootstrap.DataSecretName code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/machine_migration_helpers.go`:
- Line 92: Replace the hardcoded "worker-user-data" string with a package-level
constant (e.g., define const workerBootstrapDataSecretName = "worker-user-data")
and update the assignment to newCapiMachine.Spec.Bootstrap.DataSecretName to use
ptr.To(workerBootstrapDataSecretName); this centralizes the magic string for
easier maintenance and discovery while keeping the existing assignment in the
newCapiMachine.Spec.Bootstrap.DataSecretName code path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca9ff08d-ceeb-4f40-8e73-dc5f9ae180ea
📒 Files selected for processing (2)
e2e/machine_migration_capi_authoritative_test.goe2e/machine_migration_helpers.go
|
/test e2e-aws-capi-techpreview |
|
/assign @damdo |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo 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 |
openshift-ci
bot
added
the
approved
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
|
@sunzhaohua2: The following tests 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. |
4 participants
The first error:
2026-03-02T09:31:33Z AWSMachine/machine-auth-capi-58g4d Warning FailedGetBootstrapData failed to retrieve bootstrap data secret for AWSMachine openshift-cluster-api/machine-auth-capi-58g4d: Secret "master...Changed to use worker-user-data.
The second error:
Deleted machine.openshift.io/cluster-api-machineset label to prevent test Machines from being adopted and deleted by MachineSet controllers.
Summary by CodeRabbit
Bug Fixes
Improvements