ROSAENG-62488 | refactor: Establish target architecure for separating CLI and core/library logic - #3439
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds documentation for separating CLI presentation from reusable core logic. It defines directory responsibilities, ownership rules, boundary requirements, dependency direction, and migration guidance. It classifies Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
guidelines/ARCHITECTURE.md (1)
243-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUpdate the prohibited Survey import path to
/v2.
go.moddeclaresgithub.com/AlecAivazis/survey/v2, so this checklist still lets/v2imports through while the core-layer prohibition targets the base path.🤖 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 `@guidelines/ARCHITECTURE.md` around lines 243 - 250, Update the prohibited Survey dependency entry in the architecture checklist to use the declared github.com/AlecAivazis/survey/v2 import path, ensuring the core-layer prohibition matches go.mod and does not allow v2 imports through.
🤖 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.
Inline comments:
In `@guidelines/ARCHITECTURE.md`:
- Around line 215-228: Align the architecture terminology across both documents:
in guidelines/ARCHITECTURE.md lines 215-228, either add the permitted
internal/pkg/ → pkg/ dependency edge or remove the corresponding allowance from
the prose; in guidelines/refactor/pkg-classification.md lines 11-15, change the
private-core target from internal/ to the canonical internal/pkg/.
In `@guidelines/refactor/layer-violations.md`:
- Line 3: Correct the truncated word in the introductory paragraph by replacing
“revent” with “prevent”; leave the surrounding text unchanged.
In `@guidelines/refactor/pkg-classification.md`:
- Around line 13-15: Correct the truncated words in the classification tables:
update “egardless” to “regardless” and “oading” to “loading” in the shown rows,
and fix “tilities,” “onsumer,” and “uration” to “utilities,” “consumer,” and
“duration” in the additional affected rows.
---
Nitpick comments:
In `@guidelines/ARCHITECTURE.md`:
- Around line 243-250: Update the prohibited Survey dependency entry in the
architecture checklist to use the declared github.com/AlecAivazis/survey/v2
import path, ensuring the core-layer prohibition matches go.mod and does not
allow v2 imports through.
🪄 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: 2093a330-aad9-4a52-8bd6-452eec6c9766
📒 Files selected for processing (3)
guidelines/ARCHITECTURE.mdguidelines/refactor/layer-violations.mdguidelines/refactor/pkg-classification.md
|
@markirish I added some suggestions regarding name conflict with common packages because, since we are here refactoring, it might be a good opportunity to rename/move if makes sense. |
240b710 to
85f55ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
guidelines/ARCHITECTURE.md (1)
223-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument direct CLI imports of
internal/core/.
internal/core/is described as private implementation supportingpkg/, while the import matrix also allowscmd/andinternal/cli/to import it directly. If direct CLI use is intended outside migration work, document the exception and its scope; otherwise make the matrix rowmust not.🤖 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 `@guidelines/ARCHITECTURE.md` around lines 223 - 228, Clarify the import policy in the architecture matrix for direct imports from internal/core/ by either documenting the intended cmd/ and internal/cli/ exception and its scope, or changing those allowed cells to must not if direct CLI use is only transitional. Keep the existing pkg/core relationship unchanged.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.
Inline comments:
In `@guidelines/refactor/layer-violations.md`:
- Around line 327-343: Reconcile the aggregate totals in the layer-violations
inventory with the entries in the interactive, Cobra/pflag, and os.Exit tables.
Update the displayed totals to match the actual table counts, or explicitly
document and consistently apply a different counting rule; ensure both affected
summary sections use accurate migration-tracking figures.
In `@guidelines/refactor/pkg-architecture.md`:
- Line 93: Split the roles functionality before classifying it as CLI-only: keep
prompting, reporting, terminal checks, manual command generation, and os.Exit
behavior in internal/cli/roles, while moving reusable role creation, upgrade,
AWS operations, and trust-policy logic from pkg/helper/roles into pkg/ or
internal/core/. Update the split-work row at Line 154 to describe this
separation rather than moving the package unchanged.
---
Nitpick comments:
In `@guidelines/ARCHITECTURE.md`:
- Around line 223-228: Clarify the import policy in the architecture matrix for
direct imports from internal/core/ by either documenting the intended cmd/ and
internal/cli/ exception and its scope, or changing those allowed cells to must
not if direct CLI use is only transitional. Keep the existing pkg/core
relationship unchanged.
🪄 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: 19a379e8-626c-49b0-bbdd-54003ac2a2ff
📒 Files selected for processing (3)
guidelines/ARCHITECTURE.mdguidelines/refactor/layer-violations.mdguidelines/refactor/pkg-architecture.md
85f55ef to
b73386b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@guidelines/ARCHITECTURE.md`:
- Around line 168-180: Update the “Current State” section in ARCHITECTURE.md to
replace the inaccurate claim that the internal/ directory does not exist with a
description of its current contents and the ownership it should have in the
target architecture. Adjust any related migration guidance to match that
existing structure, while leaving the other architecture observations unchanged.
In `@guidelines/refactor/pkg-architecture.md`:
- Line 40: Update the architecture entry for pkg/helper/machinepools to classify
it as a split rather than a direct merge: move only reusable label, taint, and
tag parsing into pkg/machinepool, while retaining Cobra parameters, prompts,
reporter output, and os.Exit handling under internal/cli/ in accordance with the
project path rules.
- Around line 36-38: Update the architecture table entries for
pkg/utils/versions and pkg/download to describe parsing, comparison, error
wrapping, or structured error propagation instead of presentation or
human-readable formatting; retain terminal progress rendering under
internal/cli/. Ensure the descriptions reflect that Public Core functions return
structured data and errors rather than formatted strings.
- Line 60: The architecture classification table entry for
internal/core/httputil currently references the unclassified
internal/core/version target. Update that reference to the existing pkg/version
target, or add an explicit internal/core/version classification if that target
is intentional; keep the dependency description accurate.
🪄 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: 2c9547cf-dcab-4838-aad0-f64348f8e1f8
📒 Files selected for processing (3)
guidelines/ARCHITECTURE.mdguidelines/refactor/layer-violations.mdguidelines/refactor/pkg-architecture.md
b73386b to
7d64286
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
guidelines/refactor/layer-violations.md (1)
429-437: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReconcile the
pkg/aws/helpers.gosummary entry.The highest-impact list says
pkg/aws/helpers.gohas Cobra usage via pflag, but the Cobra and pflag tables do not list that file. Add the exact call site or remove the parenthetical. Keep the summary traceable to the inventory tables.🤖 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 `@guidelines/refactor/layer-violations.md` around lines 429 - 437, The pkg/aws/helpers.go entry in the highest-impact summary is inconsistent with the inventory tables. Update the summary and corresponding Cobra/pflag inventory so the referenced usage has an exact traceable call site, or remove the “via pflag” parenthetical if no such usage exists.
🤖 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.
Inline comments:
In `@guidelines/ARCHITECTURE.md`:
- Around line 14-18: Clarify the ownership guidance in the earlier High-Level
Shape section by explicitly labeling its bullets as the current
architecture/state, or revise them to match the target ownership defined here.
Ensure contributors can distinguish existing placement in pkg/ from the intended
CLI placement in cmd/ or internal/cli/ without conflicting rules.
In `@guidelines/refactor/layer-violations.md`:
- Around line 124-145: Update the section heading in the guidelines so it
describes all listed os.Exit usage rather than only direct calls, or separately
label the input_validation.go entry as an indirect exit reference. Keep the
existing direct-call entries and their details unchanged.
In `@guidelines/refactor/pkg-architecture.md`:
- Line 64: Update the `internal/core/policy` architecture classification to
cover only reusable policy attach/detach operations, removing manual-mode
command generation and terminal behavior from Private Core. Place manual-mode
branching and command strings in `cmd/` or `internal/cli/`, while retaining
shared business logic in `pkg/` or `internal/core/`.
- Line 61: Update the architecture table row currently labeled
`internal/core/maputil` to use the correct `internal/core/object`/`pkg/object`
target and remove the obsolete `maputil` naming, while preserving the existing
`object.Object` description.
---
Outside diff comments:
In `@guidelines/refactor/layer-violations.md`:
- Around line 429-437: The pkg/aws/helpers.go entry in the highest-impact
summary is inconsistent with the inventory tables. Update the summary and
corresponding Cobra/pflag inventory so the referenced usage has an exact
traceable call site, or remove the “via pflag” parenthetical if no such usage
exists.
🪄 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: 61d63353-8663-4a9e-a526-f8bea2920069
📒 Files selected for processing (3)
guidelines/ARCHITECTURE.mdguidelines/refactor/layer-violations.mdguidelines/refactor/pkg-architecture.md
7d64286 to
668106c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@guidelines/ARCHITECTURE.md`:
- Around line 228-251: Revise the Import Direction matrix and accompanying prose
to establish one acyclic dependency direction between pkg/ and internal/core/.
Assign shared types and constants to a single owning package, or introduce a
third shared package that neither layer reversely imports, and update all
permitted-import statements so they consistently enforce that ownership.
- Line 168: Update the os.Exit ownership row in ARCHITECTURE.md to allow both
cmd/ and internal/cli/ for CLI exits, or explicitly limit cmd/ ownership to
final process termination while including internal/cli/ runtime lifecycle exits.
🪄 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: 1563ea53-f8cf-4cac-8392-e29b6fc76114
📒 Files selected for processing (3)
guidelines/ARCHITECTURE.mdguidelines/refactor/layer-violations.mdguidelines/refactor/pkg-architecture.md
27c51c6 to
abf0e47
Compare
abf0e47 to
436ee23
Compare
|
@markirish thanks for addressing all the comments, looks great. Last one: I tested the documentation with Claude using a very simple prompt This is the output (adding just to illustrate, not related to my comment at the end) Then I asked for checking how to avoid cyclic imports, that this is my main concern. Lets see what Coderabbit has to say about it: @coderabbitai review this PR with extra attention to whether the refactoring documentation adequately addresses sibling command imports — the pattern where one cmd/ package imports another cmd/ package and calls SiblingCmd.Cmd.Flags().Set(...) followed by This pattern exists in at least 22 places across the codebase. A concrete example: cmd/create/oidcconfig/cmd.go imports cmd/create/oidcprovider and at line 303-314 mutates its flags and invokes oidcprovider.Cmd.Run() directly. The same pattern appears in cmd/create/cluster Please check:
If any of these points are missing or underspecified, flag them as suggestions for the contributor to address. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Documentation structure =='
ast-grep outline guidelines/ARCHITECTURE.md --items all 2>/dev/null || true
ast-grep outline guidelines/refactor/layer-violations.md --items all 2>/dev/null || true
ast-grep outline guidelines/refactor/pkg-architecture.md --items all 2>/dev/null || true
echo '== Documentation references to sibling commands, command imports, Cobra, and extraction order =='
rg -n -i -C 3 \
'sibling|cmd/.*cmd/|command.*import|import.*command|cobra|Flags\(\)\.Set|Cmd\.Run|extract.*first|sequenc|migration order|cyclic' \
guidelines/ARCHITECTURE.md guidelines/refactor/layer-violations.md guidelines/refactor/pkg-architecture.md || true
echo '== Concrete oidcconfig -> oidcprovider dependency =='
rg -n -C 5 \
'oidcprovider|Flags\(\)\.Set|Cmd\.Run\s*\(' \
cmd/create/oidcconfig/cmd.go || true
echo '== Sibling cmd imports and direct Cobra execution sites =='
rg -n -P -C 2 \
'"github\.com/openshift/rosa/cmd/[^"]+"' \
cmd internal/cmd 2>/dev/null || true
rg -n -P -C 3 \
'\.Cmd\.Flags\(\)\.Set\s*\(|\.Cmd\.Run\s*\(' \
cmd internal/cmd 2>/dev/null || true
echo '== cmd/create/cluster sibling imports and direct command calls =='
rg -n -C 4 \
'"github\.com/openshift/rosa/cmd/|\.Cmd\.Flags\(\)\.Set\s*\(|\.Cmd\.Run\s*\(' \
cmd/create/cluster 2>/dev/null || trueLength of output: 61077 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Changed files in this PR =='
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || \
git diff --name-status HEAD~1 HEAD
echo '== Architecture migration guidance and import-direction rules =='
nl -ba guidelines/ARCHITECTURE.md | sed -n '200,310p'
echo '== Architecture command-layer guidance =='
nl -ba guidelines/ARCHITECTURE.md | sed -n '76,145p'
echo '== Search all refactor documents that exist on this branch =='
fd -t f . guidelines/refactor -x sh -c '
echo "--- $1"
rg -n -i -C 2 "sibling|horizontal|cmd.*import|cobra.*run|flags\(\).*set|dependency|sequenc|cycle|create/cluster" "$1" || true
' sh {}
echo '== Direct cross-command calls: source file, import, flag mutation, Run call =='
rg -n -P -C 2 '\.Cmd\.(?:Flags\(\)\.Set|Run)\s*\(' cmd \
| sed -n '1,500p'Length of output: 1398 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Architecture migration guidance and import-direction rules =='
awk 'NR >= 200 && NR <= 310 { printf "%d:%s\n", NR, $0 }' guidelines/ARCHITECTURE.md
echo '== Architecture command-layer guidance =='
awk 'NR >= 76 && NR <= 145 { printf "%d:%s\n", NR, $0 }' guidelines/ARCHITECTURE.md
echo '== Existing refactor documents and references to the reported pattern =='
fd -t f . guidelines/refactor -x sh -c '
echo "--- $1"
rg -n -i -C 2 "sibling|horizontal|cmd.*import|cobra.*run|flags\(\).*set|dependency|sequenc|cycle|create/cluster" "$1" || true
' sh {}
echo '== Direct command execution and flag mutation sites =='
rg -n -P -C 2 '\.Cmd\.(?:Flags\(\)\.Set|Run)\s*\(' cmd | sed -n '1,500p'Length of output: 21684
Suggestion — document sibling command execution as a prohibited CLI coupling pattern.
The current code has this coupling:
Please add a rule near the import-direction or migration guidance sections:
This rule should distinguish command registration from command execution. Parent command registration is valid. Direct reuse of another command’s Cobra object is the coupling that the refactor must remove. You are interacting with an AI system. |
… CLI and core/library logic - Create new target repo code structure for separating architectural layers - Define responsibilities of architectural layers, including where current packages should be moved to - Define which third-party packages and function calls are forbidden in which layers - Define dependency allowed and forbidden dependency direction for repo packages - Inventory current Cobra, pflag, terminal, prompt reporter, and os.Exit boundary violations exist
436ee23 to
385071b
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, markirish 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
|
@markirish: The following test 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. |
|
/retest-required |
e82fa7d
into
openshift:master
2 participants
PR Summary
Document the target two-layer architecture for ROSA CLI and add tracking files for the incremental migration in
guidelines/refactor/.Detailed Description of the Issue
Expands the Command Layering section of guidelines/ARCHITECTURE.md from a brief three-line guideline into a full architectural specification. The new content defines the target two-layer split (CLI layer in
cmd/andinternal/cmd/; Core/Library layer inpkg/andinternal/pkg/), documents directory responsibilities, establishes boundary rules and import direction constraints, and provides a quick-reference table for where common concerns belong. It also describes the current state and sets migration guidelines for new and modified code.Two new tracking files are added under
guidelines/refactor/:layer-violations.mdcatalogs ~370 existing sites where CLI concerns have leaked into corepkg/packagespkg-classification.mdrecords the per-package classification (core vs. CLI-classified) and the split work required for each.These files are co-located with
ARCHITECTURE.mdand grouped underrefactor/so they're easy to find and easy to remove once the migration is complete.No code changes — documentation and guidelines only.
Related Issues and PRs
Type of Change
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit