[OCPERT-343] [OCPERT-344] Preserve all imports during OTE migration, fix test/e2e subdirectory logic, and update documentation links#398
Merged
openshift-merge-bot[bot] merged 4 commits intoApr 2, 2026
Conversation
…logic, and update documentation links
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDocumentation and migration workflow expanded: README clarifies automatic test migration and import preservation; SKILL.md adds required import guidance, dependency pinning and go-command flags, fixture permission fixes, broader .go file migration (including Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/ote-migration/skills/ote-migration-workflow/SKILL.md (1)
1112-1117: Consider documenting util.WithCleanup() pattern in README.The added
util.WithCleanup()call with empty function body is a clever workaround to set thetestsStartedflag for OTP functions. However, this pattern is not mentioned in the README.md.Suggestion:
Update README line 844 to mention this pattern:- **Complete Framework Initialization (2026-02-14)** - Uses `util.InitStandardFlags()`, `framework.AfterReadingAllFlags()`, `compat_otp.InitTest()`, and `util.WithCleanup()` to properly initialize the kubernetes e2e framework context and set testsStarted flag for OTP functionsThis would help users understand why this seemingly empty function call is necessary.
Also applies to: 1314-1319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ote-migration/skills/ote-migration-workflow/SKILL.md` around lines 1112 - 1117, Update the README entry around the framework initialization section to document the util.WithCleanup() pattern: mention that alongside util.InitStandardFlags(), framework.AfterReadingAllFlags(), and compat_otp.InitTest(), callers should invoke util.WithCleanup() (even with an empty callback) to set the internal testsStarted flag so OTP functions (e.g., oc.Run()/compat_otp behaviors) work correctly; add the suggested bullet text (or equivalent) under the "Complete Framework Initialization" note to explain why the empty call is necessary for kubernetes e2e framework context initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ote-migration/skills/ote-migration-workflow/SKILL.md`:
- Around line 3387-3391: Update the troubleshooting guidance to stop
recommending removal of util.WithCleanup() and keep the call that sets
testsStarted; specifically, ensure references to util.WithCleanup(func(){})
remain as the correct pattern and delete the contradicting "❌
`util.WithCleanup()` wrapper - OTE handles cleanup automatically" guidance so it
no longer advises removing util.WithCleanup; this aligns the docs with the
main.go template behavior where util.WithCleanup is used to set testsStarted =
true for OTP functions like oc.Run() to work.
- Around line 2041-2133: Step 3b conflicts with the README's preservation
promise by replacing compat_otp.NewCLI() with exutil.NewCLIWithoutNamespace();
update the README to either (A) change the preservation statement (e.g., the
line claiming "All existing imports ... are preserved as-is") to note that CLI
initialization calls are migrated, and update the examples that show
compat_otp.NewCLI() (references: README examples around lines 373, 385 and the
preservation line around 845) to use exutil.NewCLIWithoutNamespace(), or (B)
clarify the migration scope by adding a short note to Step 3b that this
replacement is intentional and applies to tests that would panic in OTE, and
ensure the step documents adding the exutil import and retaining compat_otp
helpers like By(); keep references to compat_otp.NewCLI and
exutil.NewCLIWithoutNamespace in the updated text so readers can find the
change.
---
Nitpick comments:
In `@plugins/ote-migration/skills/ote-migration-workflow/SKILL.md`:
- Around line 1112-1117: Update the README entry around the framework
initialization section to document the util.WithCleanup() pattern: mention that
alongside util.InitStandardFlags(), framework.AfterReadingAllFlags(), and
compat_otp.InitTest(), callers should invoke util.WithCleanup() (even with an
empty callback) to set the internal testsStarted flag so OTP functions (e.g.,
oc.Run()/compat_otp behaviors) work correctly; add the suggested bullet text (or
equivalent) under the "Complete Framework Initialization" note to explain why
the empty call is necessary for kubernetes e2e framework context initialization.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93626a14-f8ab-405d-ad2a-08973a882f25
📒 Files selected for processing (2)
plugins/ote-migration/README.mdplugins/ote-migration/skills/ote-migration-workflow/SKILL.md
…ement and util.WithCleanup() usage
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/ote-migration/README.md (1)
48-48:⚠️ Potential issue | 🟡 MinorClarify "preserve all existing imports" vs CLI initialization rewrite.
Line 48 states the workflow "preserves all existing imports," but this conflicts with:
- README line 385 shows
compat_otp.NewCLI()in the "After migration" example- SKILL.md Step 3b (lines 2041-2133) actively replaces
compat_otp.NewCLI()withexutil.NewCLIWithoutNamespace()- Past review comment flagged this exact inconsistency
The current wording implies imports remain untouched, but the workflow actively rewrites CLI initialization patterns. This is technically a function call modernization, not import preservation.
Suggestion: Update line 48 to clarify the scope:
-- **Automatic test migration** - Replaces FixturePath() calls, adds testdata imports, preserves all existing imports, and adds annotations atomically with rollback +- **Automatic test migration** - Replaces FixturePath() calls, adds testdata imports, preserves import statements (though some function calls like CLI initialization are modernized for OTE compatibility), and adds annotations atomically with rollbackOr update the examples at lines 373, 385 to show
exutil.NewCLIWithoutNamespace()instead ofcompat_otp.NewCLI()to match the actual post-migration behavior documented in SKILL.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ote-migration/README.md` at line 48, Update the README statement that claims "preserves all existing imports" to clarify that imports are preserved except for intentional CLI initialization rewrites (e.g., compat_otp.NewCLI() -> exutil.NewCLIWithoutNamespace()), or alternatively update the "After migration" examples (lines showing compat_otp.NewCLI()) to use exutil.NewCLIWithoutNamespace() so examples match SKILL.md Step 3b behavior; locate references to compat_otp.NewCLI() and exutil.NewCLIWithoutNamespace() in the README (examples around lines 373, 385) and SKILL.md Step 3b and either change the phrasing at line 48 to mention CLI-call modernization or replace the example CLI calls to reflect the actual migrated API.plugins/ote-migration/skills/ote-migration-workflow/SKILL.md (1)
2041-2165:⚠️ Potential issue | 🟡 MinorCLI initialization rewrite is sound but contradicts README preservation claim.
The CLI initialization modernization (Step 3b, lines 2041-2125) and unused import cleanup (Step 3c, lines 2137-2158) are technically correct:
compat_otp.NewCLI()fails in OTE due to OTP-specific initialization (lines 2127-2132 explain why)exutil.NewCLIWithoutNamespace()is the correct OTE pattern- The code properly adds/removes imports as needed
However: This directly contradicts README line 48 and 845 which claim imports are "preserved." The past review comment (lines 2041-2133) flagged this exact inconsistency.
The implementation is correct, but the documentation needs updating to reflect that CLI calls are modernized, not just preserved. See my earlier review comment on README.md line 48 for the suggested fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ote-migration/skills/ote-migration-workflow/SKILL.md` around lines 2041 - 2165, Update the README entries that claim imports/CLI initialization are "preserved" (referenced at README lines 48 and 845) to state that CLI initialization has been modernized: replace wording to indicate compat_otp.NewCLI calls are converted to exutil.NewCLIWithoutNamespace (as done in SKILL.md Step 3b) and that exutil imports may be added/removed per Step 3c; ensure the README mentions both the replacement of compat_otp.NewCLI and the cleanup of unused exutil imports so the docs match the implemented changes.
🧹 Nitpick comments (1)
plugins/ote-migration/skills/ote-migration-workflow/SKILL.md (1)
772-779: Origin version pinning prevents breaking changes.Pinning to a specific version (
v1.5.0-alpha.3.0.20260310231025-5d3fd0545b5d) instead of using@mainis a good practice to avoid unexpected breaking changes. The version is recent (March 10, 2026) relative to the PR date.Consideration: This creates a maintenance point - the pinned version should be updated periodically to incorporate upstream fixes and improvements. Consider documenting when/how to update this version, or adding a comment with the last update date.
Also applies to: 904-911
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/ote-migration/skills/ote-migration-workflow/SKILL.md` around lines 772 - 779, The script pins ORIGIN_VERSION to a specific commit tag (ORIGIN_VERSION="v1.5.0-alpha.3.0.20260310231025-5d3fd0545b5d") which is good, but you should add a short maintenance comment and update guidance next to that variable and the two go get invocations (the GOTOOLCHAIN=auto GOSUMDB=off go get "github.com/openshift/origin@$ORIGIN_VERSION" lines) so future maintainers know when/how to refresh the pin; add the last-updated date, a brief note to periodically bump ORIGIN_VERSION (or link to a documented policy), and ensure the same comment is added where the value is duplicated (also around the other occurrence referenced in the review).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/ote-migration/README.md`:
- Line 845: The README claim that "All existing import statements ... are
preserved ... and never commented out or removed" is inaccurate because SKILL.md
Phase 5 actually adds exutil imports in Step 3b and removes unused exutil
imports in Step 3c; update the README text to reflect that imports are preserved
where possible but the migration will add missing imports (see Step 3b) and
remove unused ones (see Step 3c) to keep the code compiling, and rephrase the
sentence to accurately describe this add/remove behavior rather than stating
imports are never removed.
---
Duplicate comments:
In `@plugins/ote-migration/README.md`:
- Line 48: Update the README statement that claims "preserves all existing
imports" to clarify that imports are preserved except for intentional CLI
initialization rewrites (e.g., compat_otp.NewCLI() ->
exutil.NewCLIWithoutNamespace()), or alternatively update the "After migration"
examples (lines showing compat_otp.NewCLI()) to use
exutil.NewCLIWithoutNamespace() so examples match SKILL.md Step 3b behavior;
locate references to compat_otp.NewCLI() and exutil.NewCLIWithoutNamespace() in
the README (examples around lines 373, 385) and SKILL.md Step 3b and either
change the phrasing at line 48 to mention CLI-call modernization or replace the
example CLI calls to reflect the actual migrated API.
In `@plugins/ote-migration/skills/ote-migration-workflow/SKILL.md`:
- Around line 2041-2165: Update the README entries that claim imports/CLI
initialization are "preserved" (referenced at README lines 48 and 845) to state
that CLI initialization has been modernized: replace wording to indicate
compat_otp.NewCLI calls are converted to exutil.NewCLIWithoutNamespace (as done
in SKILL.md Step 3b) and that exutil imports may be added/removed per Step 3c;
ensure the README mentions both the replacement of compat_otp.NewCLI and the
cleanup of unused exutil imports so the docs match the implemented changes.
---
Nitpick comments:
In `@plugins/ote-migration/skills/ote-migration-workflow/SKILL.md`:
- Around line 772-779: The script pins ORIGIN_VERSION to a specific commit tag
(ORIGIN_VERSION="v1.5.0-alpha.3.0.20260310231025-5d3fd0545b5d") which is good,
but you should add a short maintenance comment and update guidance next to that
variable and the two go get invocations (the GOTOOLCHAIN=auto GOSUMDB=off go get
"github.com/openshift/origin@$ORIGIN_VERSION" lines) so future maintainers know
when/how to refresh the pin; add the last-updated date, a brief note to
periodically bump ORIGIN_VERSION (or link to a documented policy), and ensure
the same comment is added where the value is duplicated (also around the other
occurrence referenced in the review).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 09ec9c1c-b25f-43f4-be76-1d11b00e265e
📒 Files selected for processing (2)
plugins/ote-migration/README.mdplugins/ote-migration/skills/ote-migration-workflow/SKILL.md
added 2 commits
March 30, 2026 17:38
…e needed, cleaning up unused imports rather than simply preserved as-is.
…mework initialization rather than incorrectly claiming it's not needed.
LuboTerifaj
approved these changes
Apr 2, 2026
| 11. **Enhanced Dependency Management** - Added retry logic and better error handling for all `go get` commands | ||
| 12. **Ginkgo Version Alignment** - Automatically aligns with OTE framework's Ginkgo version (December 2024) instead of using OTP's older version (August 2024) | ||
| 13. **Old kube-openapi Pin Removal** - Automatically removes stale kube-openapi pins from February 2024 that cause yaml type errors | ||
| 14. **Old kube-openapi Pin Removal** - Automatically removes stale kube-openapi pins from February 2024 that cause yaml type errors |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LuboTerifaj, ming1013 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
This PR improves the OTE migration workflow with three key fixes:
Preserve all existing imports during migration- Changed the test migration logic to preserve all existing imports (exutil, compat_otp, etc.) from source files rather than selectively adding/removing them. This ensures compatibility with the OTE framework and prevents import-related build failures.
Fix test/e2e subdirectory prompt logic - Corrected the conditional logic so that the subdirectory name prompt only appears when
test/e2ealready exists in the repository. Previously, the variable initialization was outside the conditional block, causing potential issues in the workflow.Update documentation links - Replaced references to deleted local markdown files (
skill-updates-summary.md,latest-skill-updates.md,OTE_PLUGIN_COMPLETE_DESIGN.md) with the canonical Confluence wiki link for the OTE migration plugin design documentation.These changes make the migration workflow more robust and prevent import-related build failures during component repository migrations.
Which issue(s) this PR fixes:
Fixes: Incorrectly removed compat_otp imports in helper/utility Go files
Fixes: Unnecessary subdirectory prompts when test/e2e doesn't exist
Fixes: Broken documentation links in README
Special notes for your reviewer:
monorepo mode tested in: openshift/router#756
single-module mode tested in: openshift/router#757
Summary by CodeRabbit