Cloud Run Action #117
Cloud Run Action #117
Conversation
| description: |- | ||
| Region in which the resource can be found. | ||
| required: false | ||
| default: us-central1 |
There was a problem hiding this comment.
Is this a conventional default region with other GCP services? Just want to make sure we're following established conventions vs defining our own.
There was a problem hiding this comment.
Yes per DevRel but not officially, I can remove and make this required?
deploy-cloudrun/src/cloudRun.ts
Outdated
| projectId = jsonContent.project_id; | ||
| core.info('Setting project Id from credentials'); | ||
| } | ||
| } else if (!projectId) { |
There was a problem hiding this comment.
It might be clearer if the assumptions are stated explicitly within the conditional statement as opposed to transitively like this. Consider something like:
if (!projectId && json.project_id) {} elseif (!projectId && process.env.GCLOUD_PROJECT) {} else {throw new Error()}
|
|
||
| // Deploy service | ||
| let serviceResponse = await client.deploy(service); | ||
| while (!serviceResponse.status!.url) { |
There was a problem hiding this comment.
What does the error messaging look like to the user with these non-null assertions? I want to be mindful of the potential errors messages which might be seen by users. I wonder if it make sense to do explicit null checks here and throughout so that we can control the error messaging.
There was a problem hiding this comment.
If there is a serviceResponse obj then there will be a status field. If that breaks then it most likely with error: TypeError: Cannot read property 'url' of undefined or TypeError: Cannot read property 'status' of undefined.
| * | ||
| * @param service Service object | ||
| */ | ||
| async delete(service: Service): Promise<void> { |
There was a problem hiding this comment.
Are we supporting a delete feature or is this just for cleaning up the test?
https://github.com/GoogleCloudPlatform/github-actions/blob/828bf4e608a7cc05a79e539b6bb9b5354bdce7eb/deploy-cloudrun/tests/unit/cloudRun.test.ts#L61
There was a problem hiding this comment.
Just for removing test resources
| * | ||
| * @param service Service object | ||
| */ | ||
| async allowUnauthenticatedRequests(service: Service): Promise<void> { |
There was a problem hiding this comment.
Not supporting, but I was keeping for prosperity. Should I remove?
No description provided.