NO-JIRA: Add test migration guide for openshift-tests-private to CVO#1336
NO-JIRA: Add test migration guide for openshift-tests-private to CVO#1336jiajliu wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jiajliu: 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. |
WalkthroughAdds a "Migrating from Custom Tags" section to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jiajliu 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)
cmd/cluster-version-operator-tests/README.md (1)
64-87: Align subsection title with actual contentLine 64 says “Handling Dropped Fields,” but Lines 66-85 describe creating custom suites with labels/CEL. Consider renaming the heading (or adding explicit dropped-field handling steps) so readers can quickly find the right guidance.
Suggested doc edit
-#### Handling Dropped Fields +#### Creating Custom Test Suites for Migration Gaps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster-version-operator-tests/README.md` around lines 64 - 87, The subsection heading "Handling Dropped Fields" is misleading relative to the content about creating custom test suites with CEL labels; update the README so the heading matches the content (for example rename it to "Creating custom test suites with labels" or "Using CEL labels to create custom suites") and keep the example showing ext.AddSuite(extension.Suite{...}) in main.go and the g.It(..., g.Label("characteristicA"), ...) test-labeling snippet; alternatively, if you intend to document dropped-field handling, add explicit steps/examples for that topic under the same heading and leave the label/CEL instructions in a new correctly named subsection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/cluster-version-operator-tests/README.md`:
- Line 48: Replace the private Google Doc link referenced in the README mapping
sentence with a public, repo-hosted source: extract or summarize the naming rule
into a new or existing public markdown file in this repository (e.g.,
docs/naming-rule.md) or link an already-public source, then update the sentence
in cmd/cluster-version-operator-tests/README.md that currently points to the
Google Doc to instead point to that public markdown file or public URL and
include a brief one-line summary of the rule inline for immediate context;
ensure the README's mapping paragraph references the new doc by its filename/URL
and removes the private Google Doc link.
---
Nitpick comments:
In `@cmd/cluster-version-operator-tests/README.md`:
- Around line 64-87: The subsection heading "Handling Dropped Fields" is
misleading relative to the content about creating custom test suites with CEL
labels; update the README so the heading matches the content (for example rename
it to "Creating custom test suites with labels" or "Using CEL labels to create
custom suites") and keep the example showing ext.AddSuite(extension.Suite{...})
in main.go and the g.It(..., g.Label("characteristicA"), ...) test-labeling
snippet; alternatively, if you intend to document dropped-field handling, add
explicit steps/examples for that topic under the same heading and leave the
label/CEL instructions in a new correctly named subsection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54ccff71-8822-4451-979f-3a82be4d4d63
📒 Files selected for processing (1)
cmd/cluster-version-operator-tests/README.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/cluster-version-operator-tests/README.md (1)
48-48:⚠️ Potential issue | 🟡 MinorThe private Google Doc link issue remains unresolved.
The reference to "this naming rule" in line 48 still appears to link to a private or inaccessible Google Doc, as flagged in the previous review. Additionally, the phrasing "based on OTP this naming rule" is grammatically awkward.
Please either:
- Move the naming rule content into this repository (e.g., in a
docs/folder), or- Link to a publicly accessible source, or
- Summarize the key rule inline in this sentence for immediate context
This will ensure all contributors can access the migration guidance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster-version-operator-tests/README.md` at line 48, The README line referencing "this naming rule" includes a private Google Doc and awkward phrasing; replace the private link and fix wording by either embedding the naming rule directly in the repo (e.g., add a short "docs/" markdown with the rule and update the README to point to it), or swap the private link for a publicly accessible source, and rephrase "based on OTP this naming rule" to something clear like "based on OTP naming rules"; update the sentence in README.md and, if you create a new docs file, reference its filename in the README to ensure contributors can access the rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/cluster-version-operator-tests/README.md`:
- Line 48: The README line referencing "this naming rule" includes a private
Google Doc and awkward phrasing; replace the private link and fix wording by
either embedding the naming rule directly in the repo (e.g., add a short "docs/"
markdown with the rule and update the README to point to it), or swap the
private link for a publicly accessible source, and rephrase "based on OTP this
naming rule" to something clear like "based on OTP naming rules"; update the
sentence in README.md and, if you create a new docs file, reference its filename
in the README to ensure contributors can access the rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f72b19a6-ea47-46de-ac74-6f26dd49b0a4
📒 Files selected for processing (1)
cmd/cluster-version-operator-tests/README.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/cluster-version-operator-tests/README.md (1)
48-48:⚠️ Potential issue | 🟡 MinorClarify or remove the “OTP naming rule” phrase.
“based on OTP naming rule” is still ambiguous (acronym/rule not defined), so this sentence can confuse external contributors.
✏️ Suggested wording
-When migrating cvo tests from [openshift-tests-private](https://github.com/openshift/openshift-tests-private/blob/main/test/extended/ota/cvo/cvo.go) that use custom-defined tags (based on OTP naming rule), use the following mapping to align with upstream test label conventions: +When migrating CVO tests from [openshift-tests-private](https://github.com/openshift/openshift-tests-private/blob/main/test/extended/ota/cvo/cvo.go) that use custom-defined tags, use the following mapping to align with upstream test label conventions:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster-version-operator-tests/README.md` at line 48, The phrase "based on OTP naming rule" is ambiguous; update the README sentence that mentions this phrase by either removing it or replacing it with an explicit description (e.g., "based on the internal OpenShift Test Platform (OTP) naming convention" or "based on the internal naming convention used in openshift-tests-private") so external contributors understand what is meant; specifically edit the sentence that currently reads "When migrating cvo tests ... that use custom-defined tags (based on OTP naming rule), use the following mapping..." to use one of the clear alternatives or drop the parenthetical entirely.
🧹 Nitpick comments (1)
cmd/cluster-version-operator-tests/README.md (1)
61-61: Prefer branch-relative link for internal utility reference.For contributor docs, a repo-relative link (or
mainbranch link) is usually easier to maintain than a commit-pinned blob URL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cluster-version-operator-tests/README.md` at line 61, Replace the commit-pinned blob URL in the README that points to test/cvo/cvo.go (the link referencing the utility functions around ConnectedOnly/NonHyperShiftHOST) with a branch-relative or repo-relative link (e.g., pointing to the file on the default branch like /blob/main/test/cvo/cvo.go or a relative path) so the reference is maintainable; update the link target text in the same README sentence to use that branch-relative path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/cluster-version-operator-tests/README.md`:
- Line 48: The phrase "based on OTP naming rule" is ambiguous; update the README
sentence that mentions this phrase by either removing it or replacing it with an
explicit description (e.g., "based on the internal OpenShift Test Platform (OTP)
naming convention" or "based on the internal naming convention used in
openshift-tests-private") so external contributors understand what is meant;
specifically edit the sentence that currently reads "When migrating cvo tests
... that use custom-defined tags (based on OTP naming rule), use the following
mapping..." to use one of the clear alternatives or drop the parenthetical
entirely.
---
Nitpick comments:
In `@cmd/cluster-version-operator-tests/README.md`:
- Line 61: Replace the commit-pinned blob URL in the README that points to
test/cvo/cvo.go (the link referencing the utility functions around
ConnectedOnly/NonHyperShiftHOST) with a branch-relative or repo-relative link
(e.g., pointing to the file on the default branch like
/blob/main/test/cvo/cvo.go or a relative path) so the reference is maintainable;
update the link target text in the same README sentence to use that
branch-relative path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2271335f-0f81-4358-a6d5-387efbfcfe9b
📒 Files selected for processing (1)
cmd/cluster-version-operator-tests/README.md
|
cc @hongkailiu @DavidHurta to review, thanks. |
|
@jiajliu: 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. |
2 participants
Added a "Migrating from Custom Tags" section for migrating tests from openshift-tests-private to align with upstream test label conventions.
NonPreRelease,Longduration,ConnectedOnly,NonHyperShiftHOST, etc.This ensures consistent migration practices and prevents CI breakages during test migration.
Summary by CodeRabbit