Add coverage for syncAvailableUpdates#1375
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: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded a new unit test that enables the FeatureGateClusterUpdateAcceptRisks feature and verifies that a previously conditional update (4.21.2) is re-evaluated into the operator’s plain available updates after calling syncAvailableUpdates. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cvo/availableupdates_test.go`:
- Around line 1193-1217: The test is seeding conditional promotion data in
ConditionalUpdates and also including the same release in upstreamUpdates, which
bypasses the upstream-based re-evaluation path exercised by
syncAvailableUpdates; update the test to put the conditional entry under
upstreamConditionalUpdates (not ConditionalUpdates) and remove the duplicate
"4.21.2" from upstreamUpdates so that syncAvailableUpdates must rebuild from
upstream* fields and actually promotes the conditional update back into Updates;
refer to upstreamConditionalUpdates, upstreamUpdates, ConditionalUpdates and
syncAvailableUpdates when making the change.
- Around line 1249-1250: The comment above the test is inaccurate relative to
the fixture: update the comment to reflect that upstreamUpdates is pre-populated
with 4.21.2 (or alternatively remove the 4.21.2 element from upstreamUpdates if
the intent was "no available updates before sync"); locate the test fixture
using the upstreamUpdates variable in pkg/cvo/availableupdates_test.go and
either correct the comment to state that an upstream update (4.21.2) exists
before sync or adjust the upstreamUpdates setup to match the original comment
intent.
🪄 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: Pro Plus
Run ID: f96955d5-03a0-47eb-a24c-a818453e78f6
📒 Files selected for processing (1)
pkg/cvo/availableupdates_test.go
|
/retest |
2 similar comments
|
/retest |
|
/retest |
Follow up openshift#1367 (comment) The commit 913e324 from openshift#1367 adds the two `upstream*` properties and use them to calculate `availableUpdates` even without fetching the updates from upstream, i.e., when `needFreshFetch==false`. It enables noticing the resolved alerts quickly. Before that change, any evaluated conditional update will stay there until `needFreshFetch==true`. This pull extends the coverage for the above behavior.
8cbe798 to
fbe0caf
Compare
|
@hongkailiu: 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. |
Follow up #1367 (comment)
The commit 913e324 from #1367 adds the two
upstream*properties and use them to calculateavailableUpdateseven without fetching the updates from upstream, i.e., whenneedFreshFetch==false. It enables noticing the resolved alerts quickly. Before that change, any evaluated conditional update will stay there untilneedFreshFetch==true.This pull extends the coverage for the above behavior.
Summary by CodeRabbit