Skip to content

feat: add user prompts for declarative security changes (PR 4)#10735

Open
inlined wants to merge 1 commit into
requires-role-3-validationfrom
requires-role-4-prompts
Open

feat: add user prompts for declarative security changes (PR 4)#10735
inlined wants to merge 1 commit into
requires-role-3-validationfrom
requires-role-4-prompts

Conversation

@inlined

@inlined inlined commented Jun 29, 2026

Copy link
Copy Markdown
Member

Description

This is PR 4 in the requiresRole PR chain.
It implements prompts asking the operator for confirmation when:

  1. Enabling declarative security (enrolling).
  2. Modifying roles on the managed service account.
  3. Opting out of declarative security (unenrolling and deleting the SA).
    Includes options.nonInteractive checks to safely throw errors in headless environments.

Scenarios Tested

  • Run unit tests: npx mocha src/deploy/functions/prompts.spec.ts
  • Run npm run build & npm run lint

Sample Commands

N/A

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the promptForSecurityChanges function in src/deploy/functions/prompts.ts along with comprehensive unit tests in src/deploy/functions/prompts.spec.ts to prompt operators for codebase-wide declarative security changes. The review feedback points out a TypeScript compilation error caused by typing the plan parameter with DeploymentPlan, which lacks the required security properties; defining a custom CodebasePlan interface is recommended to resolve this. Additionally, the feedback suggests conditionally formatting the prompt message when rolesToAdd is empty to prevent displaying an empty list of roles.

Comment thread src/deploy/functions/prompts.ts Outdated
import * as artifacts from "../../functions/artifacts";
import { Options } from "../../options";
import { EndpointUpdate } from "./release/planner";
import { EndpointUpdate, DeploymentPlan } from "./release/planner";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The DeploymentPlan type imported from ./release/planner is defined as Record<string, Changeset>, where Changeset does not contain the security-related properties (like serviceAccountToCreate, rolesToAdd, etc.) accessed in promptForSecurityChanges. This causes a TypeScript compilation error.

We should remove DeploymentPlan from this import and define a specific CodebasePlan interface to type the plan parameter correctly.

Suggested change
import { EndpointUpdate, DeploymentPlan } from "./release/planner";
import { EndpointUpdate } from "./release/planner";

Comment on lines +300 to +303
export async function promptForSecurityChanges(
plan: DeploymentPlan,
options: Options,
): Promise<void> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Define a proper CodebasePlan interface and use it to type the plan parameter. This resolves the TypeScript compilation error caused by the mismatch with DeploymentPlan (which is Record<string, Changeset>).

export interface CodebasePlan {
  serviceAccountToDelete?: string;
  serviceAccountToCreate?: string;
  managedServiceAccount?: string;
  rolesToAdd?: string[];
  rolesToRemove?: string[];
}

/**
 * Prompts operators for codebase-wide declarative security changes.
 */
export async function promptForSecurityChanges(
  plan: Record<string, CodebasePlan>,
  options: Options,
): Promise<void> {

Comment thread src/deploy/functions/prompts.ts Outdated
Comment on lines +332 to +334
const msg = `This codebase uses declarative security. It will use the following role(s):\n${roleNames
.map((r) => `* ${r}`)
.join("\n")}\nContinue?`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If rolesToAdd is empty or undefined, the prompt will print an empty list of roles, which looks awkward (e.g., It will use the following role(s):\n\nContinue?). We should conditionally format the message to only list roles if there are any.

Suggested change
const msg = `This codebase uses declarative security. It will use the following role(s):\n${roleNames
.map((r) => `* ${r}`)
.join("\n")}\nContinue?`;
let msg = `This codebase uses declarative security.`;
if (roleNames.length > 0) {
msg += ` It will use the following role(s):\n${roleNames.map((r) => `* ${r}`).join("\n")}\n`;
} else {
msg += "\n";
}
msg += "Continue?";

@inlined inlined force-pushed the requires-role-3-validation branch from 86e2632 to ef1d12f Compare June 29, 2026 16:49
@inlined inlined force-pushed the requires-role-4-prompts branch from 31fc515 to 7d9cc2f Compare June 29, 2026 16:50
@inlined inlined force-pushed the requires-role-3-validation branch from ef1d12f to 1eb74ff Compare June 29, 2026 17:33
### Description
This is PR 4 in the requiresRole PR chain.
It implements prompts asking the operator for confirmation when:
1. Enabling declarative security (enrolling).
2. Modifying roles on the managed service account.
3. Opting out of declarative security (unenrolling and deleting the SA).
Includes options.nonInteractive checks to safely throw errors in headless environments.

### Scenarios Tested
- Run unit tests: npx mocha src/deploy/functions/prompts.spec.ts
- Run npm run build & npm run lint

### Sample Commands
N/A
@inlined inlined force-pushed the requires-role-4-prompts branch from 7d9cc2f to ae9eaaa Compare June 29, 2026 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants