Conversation
|
@dis016: This pull request references OTA-1606 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 task to target the "4.22.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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-merge-robot
added
the
needs-rebase
WalkthroughAdds an end-to-end test that fetches ClusterVersion, checks Desired.Version and history Verified flag, and asserts the configured channel equals the computed stable-. prefix. Adds a utility function to compute the expected channel prefix from a semantic version. Duplicate test block present. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
|
@dis016: This pull request references OTA-1606 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 task to target the "4.22.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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dis016 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cvo/cvo.go (1)
103-115: Remove commented-out test block before merge.Keeping this large commented section adds noise and makes the suite harder to scan; prefer deleting it and relying on Git history if needed.
🧹 Proposed cleanup
- /*// Migrated from case NonHyperShiftHOST-Author:dis-High-OCP-33876-Clusterversion status has metadata stored - g.It("Clusterversion status has metadata stored", func() { - ctx := context.Background() - err := util.SkipIfHypershift(ctx, restCfg) - o.Expect(err).NotTo(o.HaveOccurred(), "Failed to determine if cluster is HyperShift") - err = util.SkipIfMicroshift(ctx, restCfg) - o.Expect(err).NotTo(o.HaveOccurred(), "Failed to determine if cluster is MicroShift") - - g.By("Checking that the Clusterversion status has metadata stored") - clusterversion, err := kubeClient.ConfigV1().ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) - o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get Clusterversion") - })*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 103 - 115, Remove the large commented-out test block that begins with "Clusterversion status has metadata stored" (the /* ... */ block containing util.SkipIfHypershift, util.SkipIfMicroshift, and the call to kubeClient.ConfigV1().ClusterVersions().Get) from test/cvo/cvo.go so the file no longer contains the dead test; rely on Git history if the code is needed later.
🤖 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/cvo/cvo.go`:
- Line 116: Fix the typos in the migrated test text and log message: locate the
comment/string containing "NonHyperShiftHOST-Author:dis-Medium-OCP-54887-The
default channel should be corret" and change "corret" to "correct", and locate
the log/test message containing the misspelled "Deafault" and change it to
"Default" so the test output and logs read correctly.
---
Nitpick comments:
In `@test/cvo/cvo.go`:
- Around line 103-115: Remove the large commented-out test block that begins
with "Clusterversion status has metadata stored" (the /* ... */ block containing
util.SkipIfHypershift, util.SkipIfMicroshift, and the call to
kubeClient.ConfigV1().ClusterVersions().Get) from test/cvo/cvo.go so the file no
longer contains the dead test; rely on Git history if the code is needed later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 442ae3f3-f023-4886-af29-412eef42c3dc
📒 Files selected for processing (2)
test/cvo/cvo.gotest/util/util.go
| o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get Clusterversion") | ||
| })*/ | ||
|
|
||
| // Migrated from case NonHyperShiftHOST-Author:dis-Medium-OCP-54887-The default channel should be corret |
There was a problem hiding this comment.
Fix typos in migrated case text and log message.
Line 116 has “corret”, and Line 148 has “Deafault”; these should be corrected for clarity in test output/logs.
✏️ Proposed wording fix
- // Migrated from case NonHyperShiftHOST-Author:dis-Medium-OCP-54887-The default channel should be corret
+ // Migrated from case NonHyperShiftHOST-Author:dis-Medium-OCP-54887-The default channel should be correct
...
- logger.Info("Deafault channel for version is correct", "version:", version, "channel:", channel)
+ logger.Info("Default channel for version is correct", "version:", version, "channel:", channel)Also applies to: 148-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cvo/cvo.go` at line 116, Fix the typos in the migrated test text and log
message: locate the comment/string containing
"NonHyperShiftHOST-Author:dis-Medium-OCP-54887-The default channel should be
corret" and change "corret" to "correct", and locate the log/test message
containing the misspelled "Deafault" and change it to "Default" so the test
output and logs read correctly.
openshift-merge-robot
removed
the
needs-rebase
|
@dis016: This pull request references OTA-1606 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 task to target the "4.22.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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/cvo/cvo.go (1)
116-148:⚠️ Potential issue | 🟡 MinorFix remaining typos in migrated text and log message.
Line 116 should use
correct, and Line 148 should useDefault.✏️ Suggested patch
- // Migrated from case NonHyperShiftHOST-Author:dis-Medium-OCP-54887-The default channel should be corret + // Migrated from case NonHyperShiftHOST-Author:dis-Medium-OCP-54887-The default channel should be correct ... - logger.Info("Deafault channel for version is correct", "version:", version, "channel:", channel) + logger.Info("Default channel for version is correct", "version:", version, "channel:", channel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 116 - 148, Update the test description string in the Ginkgo spec (the g.It call with text "The default channel should be corret") to use "correct", and fix the logger.Info message that currently reads "Deafault channel for version is correct" to "Default channel for version is correct" (refer to the g.It invocation and the logger.Info(...) call near the end of the test).
🧹 Nitpick comments (2)
test/cvo/cvo.go (2)
146-147: Add an explicit assertion for parsed expected channel.Assert
expectedChannelPrefixis non-empty before equality check so parse failures are surfaced with a clearer cause.✅ Suggested patch
expectedChannelPrefix := util.GetExpectedChannelPrefix(version) + o.Expect(expectedChannelPrefix).NotTo(o.BeEmpty(), "Failed to derive expected channel from version %s", version) o.Expect(channel).To(o.Equal(expectedChannelPrefix), "Expected channel to be %s based on version %s, but got %s", expectedChannelPrefix, version, channel)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 146 - 147, The test currently compares channel to util.GetExpectedChannelPrefix(version) without checking that the parsed expectedChannelPrefix is valid; add an explicit assertion that the value returned by util.GetExpectedChannelPrefix(version) (the variable expectedChannelPrefix) is non-empty (e.g., using o.Expect(expectedChannelPrefix).ToNot(o.BeEmpty(), "...")) before the equality assertion so parse failures surface clearly, then proceed with the existing o.Expect(channel).To(o.Equal(expectedChannelPrefix), ...) check.
103-114: Remove the commented-out test body.This block is dead code in its current form. Please remove it (or replace it with a short tracking comment/issue reference) to keep the E2E test file focused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cvo/cvo.go` around lines 103 - 114, Remove the dead, commented-out test block containing the g.It("Clusterversion status has metadata stored") body (which references ctx, util.SkipIfHypershift, util.SkipIfMicroshift, and kubeClient.ConfigV1().ClusterVersions().Get) — either delete the entire /* ... */ commented test or replace it with a concise tracking comment or issue ID; ensure no leftover commented code remains inside the cvo.go test file so the E2E test stays focused and clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/cvo/cvo.go`:
- Around line 116-148: Update the test description string in the Ginkgo spec
(the g.It call with text "The default channel should be corret") to use
"correct", and fix the logger.Info message that currently reads "Deafault
channel for version is correct" to "Default channel for version is correct"
(refer to the g.It invocation and the logger.Info(...) call near the end of the
test).
---
Nitpick comments:
In `@test/cvo/cvo.go`:
- Around line 146-147: The test currently compares channel to
util.GetExpectedChannelPrefix(version) without checking that the parsed
expectedChannelPrefix is valid; add an explicit assertion that the value
returned by util.GetExpectedChannelPrefix(version) (the variable
expectedChannelPrefix) is non-empty (e.g., using
o.Expect(expectedChannelPrefix).ToNot(o.BeEmpty(), "...")) before the equality
assertion so parse failures surface clearly, then proceed with the existing
o.Expect(channel).To(o.Equal(expectedChannelPrefix), ...) check.
- Around line 103-114: Remove the dead, commented-out test block containing the
g.It("Clusterversion status has metadata stored") body (which references ctx,
util.SkipIfHypershift, util.SkipIfMicroshift, and
kubeClient.ConfigV1().ClusterVersions().Get) — either delete the entire /* ...
*/ commented test or replace it with a concise tracking comment or issue ID;
ensure no leftover commented code remains inside the cvo.go test file so the E2E
test stays focused and clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59280b20-64c8-4604-8018-90e9775627a4
📒 Files selected for processing (2)
test/cvo/cvo.gotest/util/util.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/util/util.go
|
@dis016: 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. |
3 participants
OTA-1606: add test https://polarion.engineering.redhat.com/polarion/#/project/OSE/workitem?id=OCP-54887
/cc @jiajliu @JianLi-RH @jhou1
Summary by CodeRabbit