Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove distinction between build and run accounts #6952

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove distinction between build and run accounts #6952

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aedc01c
Remove distinction between build and run accounts
inlined Apr 2, 2024
1dba376
Formatter
inlined Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/apphosting/secrets/dialogs.ts
View file
Open in desktop
Expand Up @@ -27,7 +27,11 @@ export function toMetadata(
for (const backend of backends) {
// Splits format projects/<unused>/locations/<location>/backends/<id>
const [, , , location, , id] = backend.name.split("/");
metadata.push({ location, id, serviceAccounts: serviceAccountsForBackend(projectNumber, backend) });
metadata.push({
location,
id,
serviceAccounts: serviceAccountsForBackend(projectNumber, backend),
Copy link
Contributor

Choose a reason for hiding this comment

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

is updating this field to be a Set an option?

});
}
return metadata.sort((left, right) => {
const cmplocation = left.location.localeCompare(right.location);
Expand All @@ -39,7 +43,10 @@ export function toMetadata(
}

const matchesServiceAccounts = (target: BackendMetadata) => (test: BackendMetadata) => {
return target.serviceAccounts.length === test.serviceAccounts.length && target.serviceAccounts.every(sa => test.serviceAccounts.indexOf(sa) != -1);
return (
target.serviceAccounts.length === test.serviceAccounts.length &&
target.serviceAccounts.every((sa) => test.serviceAccounts.indexOf(sa) !== -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having BackendMetadata.serviceAccounts be a Set could help making equality checks a little more straightforward. Otherwise, we end up with an order-dependent equality check which may be unexpected

);
};

/**
Expand All @@ -52,7 +59,7 @@ export function tableForBackends(
const headers = [
"location",
"backend",
metadata.every(m => m.serviceAccounts.length === 1) ? "service account" : "service accounts",
metadata.every((m) => m.serviceAccounts.length === 1) ? "service account" : "service accounts",
];
const rows = metadata.map((m) => [m.location, m.id, m.serviceAccounts.join(", ")]);
return [headers, rows];
Expand Down Expand Up @@ -112,7 +119,8 @@ export async function selectBackendServiceAccounts(
"apphosting",
"To use this secret, your backend's service account must have secret accessor permission. All of your backends use " +
(metadata[0].serviceAccounts.length === 1 ? "service account " : "service accounts ") +
metadata[0].serviceAccounts.join(", ") + ". Granting access to one backend will grant access to all backends.",
metadata[0].serviceAccounts.join(", ") +
". Granting access to one backend will grant access to all backends.",
);
const grant = await prompt.confirm({
nonInteractive: options.nonInteractive,
Expand Down
4 changes: 2 additions & 2 deletions src/apphosting/secrets/index.ts
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ export function serviceAccountsForBackend(
if (backend.serviceAccount) {
return [backend.serviceAccount];
}
return [gcb.getDefaultServiceAccount(projectNumber), gce.getDefaultServiceAccount(projectNumber)]
return [gcb.getDefaultServiceAccount(projectNumber), gce.getDefaultServiceAccount(projectNumber)];
}

/**
Expand All @@ -31,7 +31,7 @@ export async function grantSecretAccess(
secretName: string,
accounts: string[],
): Promise<void> {
const members = accounts.map(a => `serviceAccount:${a}`);
const members = accounts.map((a) => `serviceAccount:${a}`);
const newBindings: iam.Binding[] = [
{
role: "roles/secretmanager.secretAccessor",
Expand Down
18 changes: 3 additions & 15 deletions src/test/apphosting/secrets/dialogs.spec.ts
View file
Open in desktop
Expand Up @@ -81,11 +81,7 @@ describe("dialogs", () => {
const legacyAccounts = secrets.serviceAccountsForBackend("number", legacy);
expect(table[0]).to.deep.equal(["location", "backend", "service accounts"]);
expect(table[1]).to.deep.equal([
[
"l",
"legacy",
legacyAccounts.join(", "),
],
["l", "legacy", legacyAccounts.join(", ")],
["l", "modernA", "a"],
]);
});
Expand Down Expand Up @@ -309,11 +305,7 @@ describe("dialogs", () => {
type: "checkbox",
message:
"Which service accounts would you like to grant access? Press Space to select accounts, then Enter to confirm your choices.",
choices: [
"a",
"b",
...legacyAccounts,
].sort(),
choices: ["a", "b", ...legacyAccounts].sort(),
});
expect(utils.logLabeledBullet).to.have.been.calledWith(
"apphosting",
Expand Down Expand Up @@ -341,11 +333,7 @@ describe("dialogs", () => {
type: "checkbox",
message:
"Which service accounts would you like to grant access? Press Space to select accounts, then Enter to confirm your choices.",
choices: [
"a",
"b",
...legacyAccounts,
].sort(),
choices: ["a", "b", ...legacyAccounts].sort(),
});
expect(utils.logLabeledBullet).to.have.been.calledWith(
"apphosting",
Expand Down