feat: implement security discovery and validation in prepare phase (PR 3)#10734
feat: implement security discovery and validation in prepare phase (PR 3)#10734inlined wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces declarative security coordination for Cloud Functions codebases by adding the discoverSecurityDetails function, which manages managed service accounts and etag labels. It also simplifies API enablement in ensureAllRequiredAPIsEnabled by removing the distinction between standard and additional APIs and eliminating the interactive user confirmation prompt. Feedback highlights that removing this prompt bypasses a safeguard against unexpected billing charges from enabling APIs, and questions if this change in behavior was intentional.
| export async function ensureAllRequiredAPIsEnabled( | ||
| projectNumber: string, | ||
| wantBackend: backend.Backend = backend.empty(), | ||
| options: { force?: boolean; nonInteractive?: boolean } = {}, | ||
| wantBackend: backend.Backend, | ||
| ): Promise<void> { |
There was a problem hiding this comment.
This function's behavior has been significantly changed. The previous implementation would differentiate between standard and additional APIs, and would prompt the user for confirmation before enabling any additional APIs that were not already active. This was a useful safeguard, as enabling some APIs can have billing implications.
The new implementation enables all required APIs from wantBackend.requiredAPIs without confirmation. It also now includes logic to enable GCFv2-related APIs and the Secret Manager API if needed. While centralizing API enablement is good, removing the user prompt could lead to unexpected costs for users. Was this intentional? If so, it might be worth noting this change in behavior in the PR description.
There was a problem hiding this comment.
The comments should not have been changed, but this feedback is wrong. The prompt is still below
inlined
force-pushed
the
requires-role-2-iam
branch
from
4a06f05 to
1ddf608
Compare
inlined
force-pushed
the
requires-role-3-validation
branch
from
86e2632 to
ef1d12f
Compare
inlined
force-pushed
the
requires-role-2-iam
branch
from
1ddf608 to
b4dd6a2
Compare
### Description This is PR 3 in the requiresRole PR chain. It implements deployment validation and codebase security details discovery during the preparation phase: 1. discoverSecurityDetails to identify whether a codebase is using declarative security and resolving the managed service account. 2. Ensures custom service accounts and declarative security are not combined. 3. Performs early checks for permission requirements (iam.serviceAccounts.create, resourcemanager.projects.setIamPolicy). ### Scenarios Tested - Run unit tests: npx mocha src/deploy/functions/prepare.spec.ts - Run npm run build & npm run lint ### Sample Commands N/A
inlined
force-pushed
the
requires-role-3-validation
branch
from
ef1d12f to
1eb74ff
Compare
2 participants
Description
This is PR 3 in the requiresRole PR chain.
It implements deployment validation and codebase security details discovery during the preparation phase:
discoverSecurityDetailsto identify whether a codebase is using declarative security and resolving the managed service account.iam.serviceAccounts.create,resourcemanager.projects.setIamPolicy).Scenarios Tested
npx mocha src/deploy/functions/prepare.spec.tsSample Commands
N/A