-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cloud Run No Build Initial MVP Implementation v2 #9707
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
Open
brittanycho
wants to merge
27
commits into
main
Choose a base branch
from
zip-deploy-4
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 12 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
292c48b
initial cloud run no build deploy implementation mvp
brittanycho 57855da
manual testing files - will remove later
brittanycho b4f9633
fixes formatting issues
brittanycho f24830e
Fix runv2.spec.ts concurrency expectation
brittanycho 6670e13
removing manual test files
brittanycho dc1978e
Merge branch 'master' into zip-deploy-4
brittanycho 487ad04
Fix lint errors
brittanycho 15a7b26
Update json schema for dart3
brittanycho 23c6d27
Fix lint errors and remove verify script
brittanycho 366fe34
Fix TS errors in fabricator.ts for vscode build
brittanycho e66d9de
Fixes minor formatting issues
brittanycho 1975dec
Fixed linting
brittanycho 6b53bf9
Merge branch 'master' into zip-deploy-4
brittanycho f385cc3
addresses feedback
brittanycho e98b149
Merge branch 'master' into zip-deploy-4
brittanycho 55ee19f
remove runtime config for v2 and run
brittanycho 92124c2
Merge branch 'master' into zip-deploy-4
brittanycho 2e3ceec
adds relevant comments
brittanycho f767878
Merge branch 'master' into zip-deploy-4
brittanycho cc924c4
Merge branch 'master' into zip-deploy-4
brittanycho 7a32e38
add additional fields and other minor fixes
brittanycho 0d985f2
Merge branch 'master' into zip-deploy-4
brittanycho 9a63a38
corrects test
brittanycho 13fb338
fixes error
brittanycho 4714005
fix prettier
brittanycho d6d22cb
Merge branch 'master' into zip-deploy-4
brittanycho 220df7d
Merge branch 'master' into zip-deploy-4
brittanycho File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import * as experiments from "../../experiments"; | |
| import { findEndpoint } from "./backend"; | ||
| import { deploy as extDeploy } from "../extensions"; | ||
| import { getProjectNumber } from "../../getProjectNumber"; | ||
| import * as path from "path"; | ||
|
|
||
| setGracefulCleanup(); | ||
|
|
||
|
|
@@ -51,10 +52,16 @@ async function uploadSourceV1( | |
| } | ||
|
|
||
| // Trampoline to allow tests to mock out createStream. | ||
| /** | ||
| * | ||
| */ | ||
brittanycho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
brittanycho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| export function createReadStream(filePath: string): NodeJS.ReadableStream { | ||
| return fs.createReadStream(filePath); | ||
| } | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| export async function uploadSourceV2( | ||
| projectId: string, | ||
| projectNumber: string, | ||
|
|
@@ -80,7 +87,9 @@ export async function uploadSourceV2( | |
| }; | ||
|
|
||
| // Legacy behavior: use the GCF API | ||
| if (!experiments.isEnabled("runfunctions")) { | ||
| // We use BYO bucket if the "runfunctions" experiment is enabled OR if we have any platform: run endpoints. | ||
brittanycho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Otherwise, we use the GCF API. | ||
| if (!experiments.isEnabled("runfunctions") && !v2Endpoints.some((e) => e.platform === "run")) { | ||
|
Contributor
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. it seems like we ahve 2 experiemtns being used here - suggest aligning on 1? |
||
| if (process.env.GOOGLE_CLOUD_QUOTA_PROJECT) { | ||
| logLabeledWarning( | ||
| "functions", | ||
|
|
@@ -116,7 +125,7 @@ export async function uploadSourceV2( | |
| }, | ||
| }, | ||
| }); | ||
| const objectPath = `${source.functionsSourceV2Hash}.zip`; | ||
| const objectPath = `${source.functionsSourceV2Hash}${path.extname(source.functionsSourceV2!)}`; | ||
| await gcs.upload( | ||
| uploadOpts, | ||
| `${bucketName}/${objectPath}`, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
q: why exclude run?
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.
iiuc GCF and Run have different permissions (cloudfunctions.functions.setIamPolicy vs. run.services.setIamPolicy) and the specific check where it is excluded here is for GCF permission
GCF: https://docs.cloud.google.com/functions/docs/reference/iam/permissions#functions
Run: https://docs.cloud.google.com/run/docs/reference/iam/permissions#services
For current no build mvp implementation, I think the current approach is implemented so that we try to set the policy in setInvoker (within fabricator.ts) and if it fails iiuc there should be an IAM error from Cloud Run
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.
I think we'll need similar check too, but we can leave that as todo for next PR.