Skip to content

ROSAENG-64571 | refactor: add Prompter DI for interactive account-roles tests - #3450

Closed
amandahla wants to merge 1 commit into
openshift:masterfrom
amandahla:ROSAENG-64571-prompter-poc
Closed

ROSAENG-64571 | refactor: add Prompter DI for interactive account-roles tests#3450
amandahla wants to merge 1 commit into
openshift:masterfrom
amandahla:ROSAENG-64571-prompter-poc

Conversation

@amandahla

@amandahla amandahla commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

PR Summary

Add a Prompter interface on Runtime so interactive account-roles flows (Polarion OCP-43071 steps 3–4) can be unit-tested without a real terminal. Production behavior unchanged via SurveyPrompter.

Detailed Description of the Issue

Interactive ROSA CLI flows call package-level interactive.Get* helpers backed by survey, which blocks unit tests from asserting which questions run under which flags. ROSAENG-64571 asks for a Prompter DI pilot on create account-roles so Polarion-style interactive flow checks can run in-process.

Related Issues and PRs

  • Jira: ROSAENG-64571
  • Fixes: N/A
  • Related PR(s): N/A
  • Related design/docs: guidelines/ARCHITECTURE.md, guidelines/refactor/pkg-architecture.md (interim Prompter note; target remains internal/cli/)

Type of Change

  • feat - adds a new user-facing capability.
  • fix - resolves an incorrect behavior or bug.
  • docs - updates documentation only.
  • style - formatting or naming changes with no logic impact.
  • refactor - code restructuring with no behavior change.
  • test - adds or updates tests only.
  • chore - maintenance work (tooling, housekeeping, non-product code).
  • build - changes build system, packaging, or dependencies for build output.
  • ci - changes CI pipelines, jobs, or automation workflows.
  • perf - improves performance without changing intended behavior.

Previous Behavior

create account-roles interactive prompts used package interactive.Get* directly. Classic vs hosted-cp ask/skip rules lived inline in run. Those rules were not covered by unit tests.

Behavior After This Change

  • Runtime carries Prompter (default SurveyPrompter).
  • Package Get* helpers delegate to defaultPrompter.
  • create account-roles uses r.Prompter; classic/hosted-cp flow is in promptClassicAndHostedCP.
  • Unit tests cover OCP-43071 steps 3–4 (which bool prompts run / defaults / skip on --classic / --hosted-cp).
  • CLI flags and user-visible survey behavior are intended to be unchanged.
  • Secondary commits: lowercase GetMode error string (staticcheck) + matching test assertion.

How to Test (Step-by-Step)

Preconditions

  • Go toolchain matching go.mod
  • No AWS/OCM credentials required for the unit tests below

Test Steps

  1. go test ./pkg/interactive/ ./cmd/create/accountroles/ -count=1
  2. Optional manual smoke: rosa create account-roles -i and confirm classic (default Y) then hosted-cp (default N) still appear when no flags are set; --hosted-cp / --classic still skip the other question.

Expected Results

  1. Unit tests pass, including OCP-43071 promptClassicAndHostedCP.
  2. Manual interactive flow matches previous ask/skip behavior.

Proof of the Fix

  • Screenshots: N/A
  • Videos: N/A
  • Logs/CLI output: Pre-push checks passed (fmt, build, lint, coverage, unit/integration tests). Local CodeRabbit uncommitted review reported 0 findings (untracked files later committed).
  • Other artifacts: N/A

Breaking Changes

  • No breaking changes
  • Yes, this PR introduces a breaking change (describe impact and migration plan below)

Breaking Change Details / Migration Plan

N/A

Developer Verification Checklist

  • Commit subject/title follows [JIRA-TICKET] | [TYPE]: <MESSAGE>.
  • PR description clearly explains both what changed and why.
  • Relevant Jira/GitHub issues and related PRs are linked.
  • make install-hooks has been run in this clone.
  • Tests were added/updated where appropriate.
  • I manually tested the change.
  • make test passes.
  • make lint passes.
  • make rosa passes.
  • Documentation or repo-local agent guidance was added/updated where appropriate.
  • Any risk, limitation, or follow-up work is documented.

Made with Cursor

Summary by CodeRabbit

  • New Features

    • Added interactive prompts for selecting Classic and Hosted control-plane account roles.
    • Prompts support configurable defaults and skip selections already provided through command options.
    • Interactive input can be supplied through a reusable runtime prompt interface.
    • Standardized interactive prompts for common input types while preserving validation and error handling.
  • Bug Fixes

    • Improved consistency and validation when collecting role selections, including shared VPC and ARN checks.
  • Documentation

    • Updated architecture guidance to document interactive prompting and testing patterns.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: cf9e6b84-fd8e-4fa1-8f75-fb7ffc9fc507

📥 Commits

Reviewing files that changed from the base of the PR and between e82fa7d and 7331808.

📒 Files selected for processing (11)
  • cmd/create/accountroles/cmd.go
  • cmd/create/accountroles/cmd_test.go
  • cmd/create/accountroles/interactive_prompts.go
  • cmd/create/accountroles/recording_prompter_test.go
  • guidelines/ARCHITECTURE.md
  • guidelines/refactor/pkg-architecture.md
  • pkg/interactive/interactive.go
  • pkg/interactive/mode.go
  • pkg/interactive/mode_test.go
  • pkg/interactive/prompter.go
  • pkg/rosa/runtime.go
🚧 Files skipped from review as they are similar to previous changes (11)
  • cmd/create/accountroles/cmd_test.go
  • pkg/interactive/mode.go
  • cmd/create/accountroles/recording_prompter_test.go
  • guidelines/ARCHITECTURE.md
  • pkg/interactive/prompter.go
  • pkg/interactive/mode_test.go
  • guidelines/refactor/pkg-architecture.md
  • pkg/rosa/runtime.go
  • cmd/create/accountroles/interactive_prompts.go
  • cmd/create/accountroles/cmd.go
  • pkg/interactive/interactive.go

📝 Walkthrough

Walkthrough

The interactive package adds injectable Prompter methods and retains package-level wrappers. Runtime initializes and exposes SurveyPrompter. Account-role creation uses the runtime prompter for input collection and combines Classic and Hosted CP selection. Tests cover prompt order, defaults, and flag-based prompt skipping. Architecture documentation records the interim design.

Possibly related PRs

  • openshift/rosa#3439: Documents the architecture direction advanced by the injectable Prompter integration.

Suggested reviewers: davidleerh, marcolan018

🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Test Structure And Quality ⚠️ Warning The new Ginkgo flow tests contain four Expect(err).NotTo(HaveOccurred()) assertions without failure messages, contrary to the assertion-message requirement. Add meaningful context to each prompt-flow error assertion, such as identifying the prompt scenario that failed.
✅ Passed checks (13 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the Prompter dependency-injection refactor for interactive account-roles tests and follows the required commit format.
Description check ✅ Passed The description covers the problem, rationale, changes, validation, related issue, behavior, risks, and checklist.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All added Ginkgo titles are static string literals; none include runtime values, generated identifiers, dates, IPs, namespaces, or node names.
Microshift Test Compatibility ✅ Passed The added Ginkgo tests are local account-roles unit tests; they only exercise prompt logic and inject a recording prompter, with no OpenShift APIs, namespaces, or MicroShift-incompatible assumptions.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The added Ginkgo tests exercise account-role prompt logic with an injected recording prompter; they use no cluster, node, scheduling, or HA assumptions.
Topology-Aware Scheduling Compatibility ✅ Passed The patch changes Go interactive prompting, runtime dependencies, tests, and documentation only; it adds no manifests, controllers, or topology-related scheduling constraints.
Ote Binary Stdout Contract ✅ Passed No added stdout or logging calls occur in main, init, TestMain, or suite setup; the PR only adds prompt injection and tests, while existing RunSpecs and PrintHelp code is unchanged.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The added Ginkgo tests are unit tests using a recording prompter; they contain no IPv4 literals, IP parsing, URLs, DNS, registry access, or external network calls.
No-Weak-Crypto ✅ Passed The PR changes prompting and runtime dependency injection only; added code uses no MD5, SHA1, DES, RC4, Blowfish, ECB, custom crypto, or secret comparisons.
Container-Privileges ✅ Passed The PR changes only Go and Markdown files; its commit adds no container/Kubernetes manifests or privileged, host namespace, SYS_ADMIN, or allowPrivilegeEscalation settings.
No-Sensitive-Data-In-Logs ✅ Passed The diff adds no sensitive-value logging; it only injects prompters, and the sole new Reporter.Errorf logs a generic prompt error. Existing ARN/external-ID logs are unchanged.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amandahla

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2026
…es tests

Co-authored-by: Cursor <cursoragent@cursor.com>
@amandahla
amandahla force-pushed the ROSAENG-64571-prompter-poc branch from d7d2866 to 7331808 Compare August 5, 2026 19:52
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@amandahla: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/govulncheck 7331808 link false /test govulncheck

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@amandahla

Copy link
Copy Markdown
Contributor Author

Giving up the idea considering the bubble tea will change everything/not reusable.

@amandahla amandahla closed this Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant