ROSAENG-61412 | fix: align y-stream upgrade tests with channel flow#3353
ROSAENG-61412 | fix: align y-stream upgrade tests with channel flow#3353olucasfreitas wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds y-stream upgrade preparation and next-minor upgrade discovery helpers for ROSA CLI tests, then updates multiple e2e upgrade flows to use the shared discovery path. It also adds jq bootstrap and checksum verification to 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
tests/utils/exec/rosacli/cluster_service.go (1)
53-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd doc comments to new exported symbols.
YStreamUpgradePreparationand the newPrepareClusterForYStreamUpgradeinterface method are exported but have no doc comments.As per coding guidelines, "Use exported symbol doc comments when new public types or functions are introduced."
📝 Proposed doc comments
+ // PrepareClusterForYStreamUpgrade computes the next-minor channel for the given + // channel group and applies it to the cluster if not already set. PrepareClusterForYStreamUpgrade(clusterID string, channelGroup string) (*YStreamUpgradePreparation, error) } type clusterService struct { ResourcesService } +// YStreamUpgradePreparation captures the channel information computed and applied +// while preparing a cluster for a y-stream (next-minor) upgrade. type YStreamUpgradePreparation struct {🤖 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 `@tests/utils/exec/rosacli/cluster_service.go` around lines 53 - 65, Add doc comments for the new exported symbols in the cluster service API: document the PrepareClusterForYStreamUpgrade interface method on the service interface and the YStreamUpgradePreparation type definition. Keep the comments concise and descriptive, and make sure they clearly explain the purpose of the method and the structure so they satisfy exported symbol documentation guidelines.Source: Coding guidelines
tests/utils/exec/rosacli/upgrade_service_test.go (1)
12-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining validation/error branches.
FindNextMinorUpgrade's parse-error branches, andwaitForAvailableYStreamUpgrade'sclusterID/preparationnil-guards and non-positive interval/timeout fallback, are untested. These are part of the changed lines subject to the repo's coverage gate.As per coding guidelines, "All code should be covered by tests, and Go tests in this repository must use Ginkgo."
🤖 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 `@tests/utils/exec/rosacli/upgrade_service_test.go` around lines 12 - 106, Add Ginkgo coverage for the missing validation and error branches in FindNextMinorUpgrade and waitForAvailableYStreamUpgrade. Create tests that exercise the version parse failures in UpgradeVersionList.FindNextMinorUpgrade, plus the nil-guard paths for clusterID and YStreamUpgradePreparation, and the fallback behavior when timeout/interval inputs are non-positive. Use the existing helper symbols in upgrade_service_test.go so the new cases are easy to locate and keep the assertions focused on the returned errors and fallback values.Source: Coding guidelines
tests/utils/exec/rosacli/cluster_service_test.go (1)
17-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining error branches in
prepareYStreamUpgradeChannel.Only the happy paths are tested. The
editCluster == nilerror branch, theeditClusterreturning an error branch, and the empty-channelGroupvalidation incomputeNextMinorChannelare untested. Given this repo enforces an 80% changed-line coverage gate, these missing branches risk failingcoverage-changed-files.As per coding guidelines, "All code should be covered by tests, and Go tests in this repository must use Ginkgo."
🤖 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 `@tests/utils/exec/rosacli/cluster_service_test.go` around lines 17 - 74, The current Ginkgo coverage in cluster_service_test.go only exercises happy paths for computeNextMinorChannel and prepareYStreamUpgradeChannel; add tests for the missing error branches. Cover computeNextMinorChannel rejecting an empty channelGroup, and add cases in the prepareYStreamUpgradeChannel tests that verify the nil editCluster path and the editCluster-returned-error path, using the existing helper names prepareYStreamUpgradeChannel and computeNextMinorChannel so the new specs stay aligned with the implementation.Source: Coding guidelines
tests/e2e/test_rosacli_upgrade.go (2)
711-719: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the repeated
clusterServicedeclaration into the shared setup.
clusterService := rosaClient.Clusteris now declared inline in two separateItblocks within thisDescribe. SinceupgradeServiceis already promoted to the sharedvar/BeforeEachfor this block, doing the same forclusterServicewould avoid the duplicated boilerplate.Also applies to: 788-795
🤖 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 `@tests/e2e/test_rosacli_upgrade.go` around lines 711 - 719, Hoist the repeated clusterService setup into the shared Describe-level state by declaring clusterService alongside upgradeService and initializing it in the shared BeforeEach, then update the affected It blocks to use that shared variable instead of re-declaring rosaClient.Cluster. Keep the same pattern used for upgradeService so the prepareYStreamUpgradeVersion calls in both test cases reference the shared clusterService consistently.
349-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "decide if skip this case" messaging no longer matches fail-fast behavior.
These
By(...)steps still say "to decide if skip this case," butprepareYStreamUpgradeVersionnow fails the test viaExpect(err).ToNot(HaveOccurred())rather than skipping when no upgrade target is found — consistent with the PR's stated intent to keep these lanes red. The stale wording could confuse CI failure triage. Other call sites in this same PR (e.g., line 437, 537) were updated to "Prepare cluster for y-stream upgrade target discovery."✏️ Suggested wording fix
- By("Check the cluster upgrade version to decide if skip this case") + By("Prepare cluster for y-stream upgrade target discovery") upgradingVersion, err := prepareYStreamUpgradeVersion(Also applies to: 378-389
🤖 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 `@tests/e2e/test_rosacli_upgrade.go` around lines 349 - 361, Update the stale By step messaging in the upgrade test to match the fail-fast behavior now used by prepareYStreamUpgradeVersion. In test_rosacli_upgrade.go, change the descriptive text around the calls to prepareYStreamUpgradeVersion and Expect(err).ToNot(HaveOccurred()) from “decide if skip this case” to the new “Prepare cluster for y-stream upgrade target discovery” wording used elsewhere in the same test, so the intent is consistent at the call sites around the upgrade flow.tests/utils/exec/rosacli/upgrade_service.go (2)
179-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the fallback wait interval/timeout as a named constant.
time.Secondis used as a silent fallback whenwaitInterval/waitTimeoutare non-positive. Extracting this into a named constant (e.g.,defaultYStreamPollInterval) would make the fallback behavior self-documenting and avoid a bare magic literal.As per coding guidelines, "Avoid magic numbers; extract named constants when the value matters to behavior or readability."
🤖 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 `@tests/utils/exec/rosacli/upgrade_service.go` around lines 179 - 184, The fallback behavior in the wait interval/timeout handling uses a bare time.Second literal, so extract that value into a named constant and use it in the existing waitInterval and waitTimeout checks. Update the logic in the relevant helper to reference the new constant (for example, defaultYStreamPollInterval) so the default polling behavior is self-documenting and easier to maintain.Source: Coding guidelines
165-237: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
context.Contextfor the polling/timeout loop instead of manual deadline tracking.
waitForAvailableYStreamUpgradeimplements its own deadline+sleep loop. Usingcontext.Contextwithcontext.WithTimeout(andctx.Done()/ctx.Err()) is the idiomatic Go pattern for cancellation and timeouts and would let callers cancel the wait early (e.g., on test suite interruption) instead of always blocking to the fixed deadline.As per coding guidelines, Go security requirements call for "context.Context for cancellation and timeouts."
🤖 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 `@tests/utils/exec/rosacli/upgrade_service.go` around lines 165 - 237, `waitForAvailableYStreamUpgrade` currently uses a manual deadline and sleep loop for polling, which should be replaced with `context.Context`-based cancellation and timeout handling. Update the function to accept a context (or derive one with timeout from the caller), use `context.WithTimeout` plus `ctx.Done()` in the loop instead of `time.Now().After`/`time.Sleep`, and return promptly on cancellation via `ctx.Err()` while keeping the existing fetch/version lookup logic intact.Source: Path instructions
🤖 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 `@tests/e2e/test_rosacli_upgrade.go`:
- Around line 711-719: Hoist the repeated clusterService setup into the shared
Describe-level state by declaring clusterService alongside upgradeService and
initializing it in the shared BeforeEach, then update the affected It blocks to
use that shared variable instead of re-declaring rosaClient.Cluster. Keep the
same pattern used for upgradeService so the prepareYStreamUpgradeVersion calls
in both test cases reference the shared clusterService consistently.
- Around line 349-361: Update the stale By step messaging in the upgrade test to
match the fail-fast behavior now used by prepareYStreamUpgradeVersion. In
test_rosacli_upgrade.go, change the descriptive text around the calls to
prepareYStreamUpgradeVersion and Expect(err).ToNot(HaveOccurred()) from “decide
if skip this case” to the new “Prepare cluster for y-stream upgrade target
discovery” wording used elsewhere in the same test, so the intent is consistent
at the call sites around the upgrade flow.
In `@tests/utils/exec/rosacli/cluster_service_test.go`:
- Around line 17-74: The current Ginkgo coverage in cluster_service_test.go only
exercises happy paths for computeNextMinorChannel and
prepareYStreamUpgradeChannel; add tests for the missing error branches. Cover
computeNextMinorChannel rejecting an empty channelGroup, and add cases in the
prepareYStreamUpgradeChannel tests that verify the nil editCluster path and the
editCluster-returned-error path, using the existing helper names
prepareYStreamUpgradeChannel and computeNextMinorChannel so the new specs stay
aligned with the implementation.
In `@tests/utils/exec/rosacli/cluster_service.go`:
- Around line 53-65: Add doc comments for the new exported symbols in the
cluster service API: document the PrepareClusterForYStreamUpgrade interface
method on the service interface and the YStreamUpgradePreparation type
definition. Keep the comments concise and descriptive, and make sure they
clearly explain the purpose of the method and the structure so they satisfy
exported symbol documentation guidelines.
In `@tests/utils/exec/rosacli/upgrade_service_test.go`:
- Around line 12-106: Add Ginkgo coverage for the missing validation and error
branches in FindNextMinorUpgrade and waitForAvailableYStreamUpgrade. Create
tests that exercise the version parse failures in
UpgradeVersionList.FindNextMinorUpgrade, plus the nil-guard paths for clusterID
and YStreamUpgradePreparation, and the fallback behavior when timeout/interval
inputs are non-positive. Use the existing helper symbols in
upgrade_service_test.go so the new cases are easy to locate and keep the
assertions focused on the returned errors and fallback values.
In `@tests/utils/exec/rosacli/upgrade_service.go`:
- Around line 179-184: The fallback behavior in the wait interval/timeout
handling uses a bare time.Second literal, so extract that value into a named
constant and use it in the existing waitInterval and waitTimeout checks. Update
the logic in the relevant helper to reference the new constant (for example,
defaultYStreamPollInterval) so the default polling behavior is self-documenting
and easier to maintain.
- Around line 165-237: `waitForAvailableYStreamUpgrade` currently uses a manual
deadline and sleep loop for polling, which should be replaced with
`context.Context`-based cancellation and timeout handling. Update the function
to accept a context (or derive one with timeout from the caller), use
`context.WithTimeout` plus `ctx.Done()` in the loop instead of
`time.Now().After`/`time.Sleep`, and return promptly on cancellation via
`ctx.Err()` while keeping the existing fetch/version lookup logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9210d02d-e098-4b47-93c6-5033b98a4a49
📒 Files selected for processing (5)
tests/e2e/test_rosacli_upgrade.gotests/utils/exec/rosacli/cluster_service.gotests/utils/exec/rosacli/cluster_service_test.gotests/utils/exec/rosacli/upgrade_service.gotests/utils/exec/rosacli/upgrade_service_test.go
Add shared y-stream upgrade preparation helpers that compute and apply the next-minor channel, then wait for a next-minor upgrade target to appear before the tests proceed. This aligns the e2e upgrade cases with the documented ROSA upgrade flow, removes duplicated channel setup, and keeps dedicated upgrade lanes red when no real upgrade path can be exercised.
olucasfreitas
force-pushed
the
ROSAENG-61412/fix-y-stream-upgrade-flow
branch
from
9bdaad3 to
519ffdd
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas 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 |
|
@olucasfreitas: 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. |
1 participant
Summary
Add shared y-stream upgrade preparation helpers that compute and apply the next-minor channel, then wait for a next-minor upgrade target to appear before the tests proceed.
This aligns the e2e upgrade cases with the documented ROSA upgrade flow, removes duplicated channel setup, and keeps dedicated upgrade lanes red when no real upgrade path can be exercised.
Supersedes #3343.
Detailed Description of the Issue
Jira: ROSAENG-61412
The previous change in #3343 addressed the symptom (
rosa list upgradereturning an empty list) by skipping the dedicated non-STS upgrade lane. Amanda correctly pointed out that this would let the lane go green without actually exercising an upgrade.The deeper issue is that the y-stream cases were not consistently following the documented ROSA flow for minor upgrades: the cluster needs to move to the next-minor channel before the tests ask for available upgrade targets. Some cases changed the channel first, others did not, and the duplicated logic also skipped the channel update entirely when the cluster reported an empty channel.
This PR introduces a shared preparation flow that:
y+1channel from the profile channel group and cluster raw version,rosa list upgrade, andType of Change
Previous Behavior
Y-stream upgrade tests had inconsistent setup:
rosa list upgradedirectly and assumed targets were already available,CD.Channel != "",Behavior After This Change
All affected y-stream upgrade cases now use one shared preparation flow before listing or scheduling upgrades. Dedicated upgrade lanes remain red when they cannot discover a real next-minor target after channel preparation, while shared setup logic is centralized and covered by focused tests.
Testing
make fmtmake fmt-checkmake lintmake testmake rosaGOTOOLCHAIN=auto go test ./tests/e2e/... -run TestDoesNotExist -count=0GOTOOLCHAIN=auto go test ./tests/utils/exec/rosacli/... -run TestDoesNotExist -count=0Summary by CodeRabbit
jq, and enhanced the govulncheck helper to download/verify a pinnedjqbinary.