FAH uses ids using the same naming policy as rolloutPolicy #6743
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #6743 +/- ##
===========================================
+ Coverage 0 54.26% +54.26%
===========================================
Files 0 347 +347
Lines 0 24197 +24197
Branches 0 5005 +5005
===========================================
+ Hits 0 13131 +13131
- Misses 0 9864 +9864
- Partials 0 1202 +1202 ☔ View full report in Codecov by Sentry. |
src/gcp/apphosting.ts
Outdated
| res.builds.splice(res.builds.length, 0, ...(int.body.builds || [])); | ||
| res.unreachable?.splice(res.unreachable.length, 0, ...(int.body.unreachable || [])); |
There was a problem hiding this comment.
Is it cleaner to use .push?
| res.builds.splice(res.builds.length, 0, ...(int.body.builds || [])); | |
| res.unreachable?.splice(res.unreachable.length, 0, ...(int.body.unreachable || [])); | |
| res.builds.push(...(int.body.builds || [])); | |
| res.unreachable?.push(...(int.body.unreachable || [])); |
src/gcp/apphosting.ts
Outdated
| res.rollouts.splice(res.rollouts.length, 0, ...(int.body.rollouts || [])); | ||
| res.unreachable?.splice(res.unreachable.length, 0, ...(int.body.unreachable || [])); |
There was a problem hiding this comment.
Similar to above... is push more clear?
src/gcp/apphosting.ts
Outdated
| backendId: string, | ||
| ): Promise<ListBuildsResponse> { | ||
| const name = `projects/${projectId}/locations/${location}/backends/${backendId}/builds`; | ||
| let pageToken: string | undefined = undefined; |
There was a problem hiding this comment.
Technically I don't think you need to specify = undefined - that can be dropped.
| let pageToken: string | undefined = undefined; | |
| let pageToken: string | undefined; |
src/gcp/apphosting.ts
Outdated
| /** | ||
| * Generates the next build ID to fit with the naming scheme of the backend API. | ||
| * @param counter Overrides the counter to use, avoiding an API call. | ||
| * @return |
There was a problem hiding this comment.
I think you can drop an empty @return line - though I'm not certain
src/gcp/apphosting.ts
Outdated
| // Note: month is 0 based in JS | ||
| const month = String(date.getUTCMonth()).padStart(2, "0"); |
There was a problem hiding this comment.
So should you be adding 1?
There was a problem hiding this comment.
😱 I modified the code and dropped the +1. Good catch and thank god for comments.
| } | ||
|
|
||
| // Note: must use exports here so that listRollouts can be stubbed in tests. | ||
| const builds = await (exports as { listRollouts: typeof listRollouts }).listRollouts( |
There was a problem hiding this comment.
Huh, but exports.listRollouts gives you type errors or something? That's unfortunate :(
src/test/gcp/apphosting.spec.ts
Outdated
| return `build-${year}-${month}-${day}`; | ||
| } | ||
|
|
||
| it("Should handle explicit counters", async () => { |
There was a problem hiding this comment.
These generally shouldn't be capitalized (though I'm not sure I have any style guidance to point to). When I see this, I read it as a sentence, and "it Should do this" looks abnormal (compared to "it should do this")
Additionally added listRollouts and fixed listBuilds so that it paginates over multiple requests and preserves the list of unavailable regions (and handles them)