CNTRLPLANE-3646: make lifecycle binaries configurable for non-CI environments - #9021
CNTRLPLANE-3646: make lifecycle binaries configurable for non-CI environments#9021ironcladlou wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ironcladlou: 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. |
|
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:
📝 WalkthroughWalkthroughThe v2 E2E lifecycle helpers now filter cluster specifications and test matrices by configured variants. Guest commands derive namespace, infrastructure ID, base-domain, and binary settings from environment variables. The test runner supports binary path, label filter, and namespace overrides for parallel and sequential executions. Lifecycle tests cover name derivation, variant filtering, and matrix filtering. Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/testing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/v2/lifecycle/platform.go (2)
124-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreallocate slice capacity and check map existence explicitly.
As per coding guidelines, preallocate slice capacity when the maximum size is known ahead of time to avoid runtime reallocations. Additionally, check map existence with the two-value assignment (
val, ok := m[key]) rather than relying on implicit zero-value evaluation.♻️ Proposed refactor
- var filtered []ClusterSpec + filtered := make([]ClusterSpec, 0, len(specs)) for _, s := range specs { - if allowed[s.Variant] { + if _, ok := allowed[s.Variant]; ok { filtered = append(filtered, s) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/lifecycle/platform.go` around lines 124 - 128, Update the filtering loop over specs to preallocate filtered with capacity len(specs), and use an explicit two-value lookup for allowed[s.Variant], appending only when the key exists and its value is true.Source: Coding guidelines
140-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreallocate slice capacity and check map existence explicitly.
As per coding guidelines, preallocate slice capacity when the maximum size is known ahead of time, and check map existence with the two-value assignment (
val, ok := m[key]).♻️ Proposed refactor
- var parallel []TestGroup + parallel := make([]TestGroup, 0, len(matrix.Parallel)) for _, g := range matrix.Parallel { - if clusterFiles[g.ClusterFile] { + if _, ok := clusterFiles[g.ClusterFile]; ok { parallel = append(parallel, g) } } - var sequential []SequentialGroup + sequential := make([]SequentialGroup, 0, len(matrix.Sequential)) for _, sg := range matrix.Sequential { - var steps []TestGroup + steps := make([]TestGroup, 0, len(sg.Steps)) for _, step := range sg.Steps { - if clusterFiles[step.ClusterFile] { + if _, ok := clusterFiles[step.ClusterFile]; ok { steps = append(steps, step) } } if len(steps) > 0 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/v2/lifecycle/platform.go` around lines 140 - 154, Update the filtering logic around the parallel and sequential group construction to preallocate parallel and steps slice capacities from their source lengths, and replace direct clusterFiles map lookups with two-value assignments that explicitly verify key existence and value. Preserve the existing filtering and non-empty sequential-group behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/e2e/v2/lifecycle/platform.go`:
- Around line 124-128: Update the filtering loop over specs to preallocate
filtered with capacity len(specs), and use an explicit two-value lookup for
allowed[s.Variant], appending only when the key exists and its value is true.
- Around line 140-154: Update the filtering logic around the parallel and
sequential group construction to preallocate parallel and steps slice capacities
from their source lengths, and replace direct clusterFiles map lookups with
two-value assignments that explicitly verify key existence and value. Preserve
the existing filtering and non-empty sequential-group behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 63ee991d-fc9f-486a-be35-5b2818a07d01
📒 Files selected for processing (5)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/platform.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9021 +/- ##
==========================================
+ Coverage 44.44% 44.50% +0.05%
==========================================
Files 774 774
Lines 96977 96980 +3
==========================================
+ Hits 43105 43164 +59
+ Misses 50897 50828 -69
- Partials 2975 2988 +13 see 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
5391c57 to
f8f1a0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/cmd/destroy-guests/main.go`:
- Around line 58-66: Replace the direct os.Getenv calls in the destroy-guests
configuration setup with GetEnvVarValue, and register HYPERSHIFT_NAMESPACE with
its default plus HYPERSHIFT_BASE_DOMAIN, HYPERSHIFT_INFRA_ID, and
HYPERSHIFT_VARIANTS through the centralized environment variable catalog using
RegisterEnvVar or RegisterEnvVarWithDefault. Preserve the existing
defaultNamespace fallback behavior and cluster-spec filtering.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa56a31b-e208-45c1-9aa4-04ed7d0b4591
📒 Files selected for processing (5)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/platform.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/v2/cmd/dump-guests/main.go
- test/e2e/v2/cmd/create-guests/main.go
- test/e2e/v2/cmd/run-tests/main.go
- test/e2e/v2/lifecycle/platform.go
f8f1a0e to
7760d00
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/cmd/create-guests/main.go`:
- Around line 123-125: Register HYPERSHIFT_VARIANTS,
HYPERSHIFT_INFRA_ID_FROM_NAME, and HYPERSHIFT_BINARY in
test/e2e/v2/internal/env_vars.go, using hypershift as the HYPERSHIFT_BINARY
default. In create-guests/main.go and dump-guests/main.go, replace os.Getenv and
envOrDefault lookups for these variables with GetEnvVarValue, preserving the
existing boolean conversion for HYPERSHIFT_INFRA_ID_FROM_NAME; update all three
listed sites accordingly.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7952a1b4-3571-4733-b57e-f439a82548f2
📒 Files selected for processing (5)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/platform.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/v2/cmd/destroy-guests/main.go
- test/e2e/v2/cmd/run-tests/main.go
- test/e2e/v2/lifecycle/platform.go
ironcladlou
changed the title
|
@ironcladlou: This pull request references CNTRLPLANE-3646 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
7760d00 to
b5e7582
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/v2/cmd/run-tests/main.go`:
- Line 89: Rename the goroutine-local error variable in runTestBinary calls to
avoid shadowing the outer err: update both sites in
test/e2e/v2/cmd/run-tests/main.go at lines 89-89 and 112-112, using a distinct
name such as runErr and updating all subsequent references within each closure.
In `@test/e2e/v2/lifecycle/platform_test.go`:
- Around line 44-85: Update test/e2e/v2/lifecycle/platform_test.go:44-85, 9-35,
and 104-155 to use “When … it should …” descriptions for every
FilterClusterSpecs-related case, wrapping the inline assertions at 9-35 in named
subtests. Prefer consolidating these cases into table-driven tests while
preserving all existing assertions and expected behavior.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3109e7f1-5f61-408d-bc17-903f483e3daf
📒 Files selected for processing (6)
test/e2e/v2/cmd/create-guests/main.gotest/e2e/v2/cmd/destroy-guests/main.gotest/e2e/v2/cmd/dump-guests/main.gotest/e2e/v2/cmd/run-tests/main.gotest/e2e/v2/lifecycle/platform.gotest/e2e/v2/lifecycle/platform_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/v2/cmd/dump-guests/main.go
- test/e2e/v2/lifecycle/platform.go
- test/e2e/v2/cmd/create-guests/main.go
- test/e2e/v2/cmd/destroy-guests/main.go
b5e7582 to
c52bd64
Compare
|
/retest |
1 similar comment
|
/retest |
|
/refresh |
|
/pipeline required |
|
Scheduling tests matching the |
c52bd64 to
5debabd
Compare
Test Resultse2e-aws
e2e-aks
|
|
/retest |
|
@ironcladlou: 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. |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Review from the hypershift-staff-engineer agent (Standards + Spec axes).
Spec: All four stated requirements are implemented correctly. No gaps, no concerning scope creep.
Standards: One actionable fix (test naming), plus several suggestions for reducing duplication across the four cmd/ binaries. Details inline.
| wantEmpty bool | ||
| }{ | ||
| { | ||
| name: "When the same inputs are provided it should return the same name", |
There was a problem hiding this comment.
nit (TESTING.md): Test case names need a comma after the condition clause. The documented format is "When <condition>, it should <expected behavior>".
| name: "When the same inputs are provided it should return the same name", | |
| name: "When the same inputs are provided, it should return the same name", |
Same fix needed on all 12 test cases in this file.
|
|
||
| func destroyCluster(hypershiftBin, name, variant string, platform lifecycle.PlatformConfig) error { | ||
| log.Printf("Destroying cluster %s (%s)", name, variant) | ||
| func destroyCluster(hypershiftBin, name, namespace, baseDomain string, infraIDFromName bool, platform lifecycle.PlatformConfig) error { |
There was a problem hiding this comment.
suggestion: destroyCluster now takes 6 positional params. Three of them (namespace, baseDomain, infraIDFromName) are the same env-derived config that create-guests already bundles into its envConfig struct. A shared config type (or adding these to PlatformConfig) would reduce the parameter list and keep the two binaries in sync.
Not blocking — just noting the data-clump smell before it grows.
There was a problem hiding this comment.
Seems worth a followup refactor
| const clusterGracePeriod = "40m" | ||
| const ( | ||
| clusterGracePeriod = "40m" | ||
| defaultNamespace = "clusters" |
There was a problem hiding this comment.
suggestion: The default namespace "clusters" is now defined independently in three places with two different names:
defaultNamespacehere in destroy-guestsdefaultClusterNSin run-tests- implicitly via
envConfig.namespacedefault in create-guests
Consider extracting a single DefaultNamespace constant in the lifecycle package — all four binaries already import it, and it lives next to the other helpers this PR adds (FilterClusterSpecs, FilterTestMatrix, DeriveClusterName).
|
|
||
| matrix := platform.TestMatrix(releaseImage) | ||
| variants := os.Getenv("HYPERSHIFT_VARIANTS") | ||
| specs := lifecycle.FilterClusterSpecs(platform.ClusterSpecs(releaseImage, os.Getenv("OCP_IMAGE_N1")), variants) |
There was a problem hiding this comment.
observation (not blocking): specs is reconstructed here via platform.ClusterSpecs() independently of what create-guests used at cluster creation time. If env state differs between the two invocations (e.g., OCP_IMAGE_N1 set during create but absent during test), FilterTestMatrix could silently drop valid test groups.
Might be worth a brief comment here noting the assumption that env vars must be consistent across all four binaries.
There was a problem hiding this comment.
Wouldn't this general consistency issue be applicable to every environment variable shared amongst the binaries? Not sure it's worth calling out this single instance. What may be lacking is a stable non-environment input passed through to all of the binaries if there's some concern of environment drift during the same workflow, or something. Seems like a systemic thing to follow up on?
| } | ||
|
|
||
| specs := platform.ClusterSpecs("", "") | ||
| variants := os.Getenv("HYPERSHIFT_VARIANTS") |
There was a problem hiding this comment.
nit: The pattern os.Getenv("HYPERSHIFT_VARIANTS") + lifecycle.FilterClusterSpecs(platform.ClusterSpecs(...), variants) is repeated identically in all four cmd/ binaries. Similarly, os.Getenv("HYPERSHIFT_INFRA_ID_FROM_NAME") == "true" appears in both create-guests and destroy-guests.
Fine for now, but if more env vars get added across all four binaries, a shared config loader in lifecycle/ would help.
There was a problem hiding this comment.
Agree all this stuff needs cleaned up into a structure with uniform env handling applied and passed around
…ments Remove hard-coded assumptions from the v2 lifecycle binaries so they can be driven by external tooling outside of CI step registry jobs. - Add variant filtering, namespace, base domain, binary path, and label filter override via environment variables - Pass --namespace to create/destroy CLI commands - Support deterministic infra IDs via HYPERSHIFT_INFRA_ID_FROM_NAME - Add FilterClusterSpecs and FilterTestMatrix helpers to lifecycle pkg
5debabd to
7143d3c
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, ironcladlou 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
|
Superseded by #9174 /close |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
@ironcladlou: Closed this PR. 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. |
5 participants
Remove hard-coded assumptions from the v2 lifecycle binaries so they
can be driven by external tooling outside of CI step registry jobs.
label filter override via environment variables
Part of CNTRLPLANE-3646
Summary by CodeRabbit
Summary by CodeRabbit
New Features
HYPERSHIFT_VARIANTS.--namespace(fromHYPERSHIFT_NAMESPACE, defaultclusters), and conditionally include--infra-id(viaHYPERSHIFT_INFRA_ID_FROM_NAME) and--base-domain(viaHYPERSHIFT_BASE_DOMAIN).HYPERSHIFT_BINARY,E2EV2_BIN_DIR, andGINKGO_LABEL_FILTERoverrides.Tests
e2ev2build tag.