-
Notifications
You must be signed in to change notification settings - Fork 1k
Add support for deploying local source to App Hosting #8516
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.
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
Changes from all commits
944ab1f
a0f00ff
1588252
f7be953
b5519ac
e344fae
68caf08
588b11f
8cc675f
219b901
cbe7ffe
908eb34
d771ce8
affa033
bc0cfbf
41ca746
f0eefcc
f84ff77
9fe4133
d019b57
93db1a6
e72954e
57b9d4c
23c3e2f
d6e37bc
611dc97
1526d3d
a999b7a
09bcac9
df7042d
f8cce50
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ const FILTERABLE_TARGETS = new Set([ | |
"storage", | ||
"database", | ||
"dataconnect", | ||
"apphosting", | ||
]); | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
"remoteconfig", | ||
"extensions", | ||
"dataconnect", | ||
"apphosting", | ||
]; | ||
export const TARGET_PERMISSIONS: Record<(typeof VALID_DEPLOY_TARGETS)[number], string[]> = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does apphosting require any permissions we need to check for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added calls with |
||
database: ["firebasedatabase.instances.update"], | ||
|
@@ -98,14 +99,14 @@ | |
) | ||
.before(requireConfig) | ||
.before((options) => { | ||
options.filteredTargets = filterTargets(options, VALID_DEPLOY_TARGETS); | ||
Check warning on line 102 in src/commands/deploy.ts
|
||
const permissions = options.filteredTargets.reduce((perms: string[], target: string) => { | ||
Check warning on line 103 in src/commands/deploy.ts
|
||
return perms.concat(TARGET_PERMISSIONS[target]); | ||
}, []); | ||
return requirePermissions(options, permissions); | ||
}) | ||
.before((options) => { | ||
if (options.filteredTargets.includes("functions")) { | ||
return checkServiceAccountIam(options.project); | ||
} | ||
}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { AppHostingSingle } from "../../firebaseConfig"; | ||
|
||
export interface Context { | ||
backendConfigs: Map<string, AppHostingSingle>; | ||
backendLocations: Map<string, string>; | ||
backendStorageUris: Map<string, string>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment in the code explaining why this new roles/storage.objectViewer stuff is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ for better code comprehension e.g. letting reader know that deploy from source has the source code bundled in storage buckets so we need access to that bucket.
but also, now I see below that
roles/storage.objectViewer
is also added in provisionDefaultComputeServiceAccount() below, which is called in this ensureAppHostingComputeServiceAccount()and I'm not sure why?
either there's a better way to do it to make things more clear natively or let's document what we're doing and explain any weird/confusing stuff.