test: run the bigtable system tests in google cloud build#8246
test: run the bigtable system tests in google cloud build#8246danieljbruce wants to merge 35 commits into
Conversation
…com/googleapis/google-cloud-node into migrate-bigtable-to-gcp-cloud-build
…com/googleapis/google-cloud-node into migrate-bigtable-to-gcp-cloud-build
…com/googleapis/google-cloud-node into migrate-bigtable-to-gcp-cloud-build
…com/googleapis/google-cloud-node into migrate-bigtable-to-gcp-cloud-build
…com/googleapis/google-cloud-node into migrate-bigtable-to-gcp-cloud-build
…com/googleapis/google-cloud-node into migrate-bigtable-to-gcp-cloud-build
…com/googleapis/google-cloud-node into migrate-bigtable-to-gcp-cloud-build
There was a problem hiding this comment.
Code Review
This pull request introduces a new Cloud Build configuration for Bigtable and addresses build-related issues in the tabular API surface and pack-n-play tests. Key changes include defining a local type for backoff settings to avoid problematic imports from google-gax and updating the pack-n-play test configuration to ensure proper dependency resolution. The review feedback suggests improving the Cloud Build configuration by avoiding runtime npm modifications, removing hardcoded project IDs, and optimizing build timeouts. Additionally, the reviewer recommends renaming the newly introduced type to 'BackoffSettings' to improve code clarity and consistency.
| npm install -g npm@latest | ||
| npm config set prefix /usr/local |
There was a problem hiding this comment.
Updating npm to the latest version and changing the global prefix in every build can lead to non-deterministic behavior and increased build times. It is generally recommended to use the version of npm provided by the builder image to ensure consistency across builds. If a newer version of Node.js or npm is required, consider using a specific image tag (e.g., node:18) instead of modifying the environment at runtime. This aligns with the repository's support for Node.js v18+.
References
- Use of APIs available in Node.js v18+ is permitted as older versions are not supported across the repository.
| # Define a substitution variable for your project ID | ||
| # Replace 'long-door-651' with the actual GCP Project ID your system tests should run against. | ||
| substitutions: | ||
| _GCP_PROJECT_ID: 'long-door-651' |
There was a problem hiding this comment.
The project ID long-door-651 is hardcoded in the substitutions. This makes the configuration less portable. Consider using a more generic placeholder or relying on the ${PROJECT_ID} environment variable if the tests are intended to run in the project hosting the build. If this is a dedicated test project, it's better to manage this value via Cloud Build trigger substitutions rather than hardcoding it in the YAML file.
_GCP_PROJECT_ID: 'your-project-id'| substitutions: | ||
| _GCP_PROJECT_ID: 'long-door-651' | ||
|
|
||
| timeout: '10800s' |
There was a problem hiding this comment.
The timeout of 10800s (3 hours) is exceptionally long. Unless the Bigtable system tests are known to take several hours, it is safer to set a lower timeout (e.g., 3600s for 1 hour) to prevent runaway builds from consuming excessive resources in case of a hang.
timeout: '3600s'| // StrictLongRunningType introduces a type equal to BackoffSettings so that we | ||
| // don't have to reach into the gax folder of build which is causing pack and | ||
| // play errors. | ||
| type StrictLongRunningType = NonNullable<CallOptions['longrunning']>; |
There was a problem hiding this comment.
The name StrictLongRunningType is somewhat confusing as these settings are used for general backoff/retry logic, not just long-running operations. Since this type is intended to replace the BackoffSettings import, consider naming it BackoffSettings locally. This would also reduce the number of changes needed in the rest of the file.
| // StrictLongRunningType introduces a type equal to BackoffSettings so that we | |
| // don't have to reach into the gax folder of build which is causing pack and | |
| // play errors. | |
| type StrictLongRunningType = NonNullable<CallOptions['longrunning']>; | |
| // BackoffSettings is defined here using CallOptions to avoid reaching into | |
| // the gax folder of build which is causing pack and play errors. | |
| type BackoffSettings = NonNullable<CallOptions['longrunning']>; |
| export const IGNORED_STATUS_CODES = new Set([1]); | ||
|
|
||
| export const DEFAULT_BACKOFF_SETTINGS: BackoffSettings = { | ||
| export const DEFAULT_BACKOFF_SETTINGS: StrictLongRunningType = { |
There was a problem hiding this comment.
Use the more descriptive BackoffSettings type name if the suggestion to rename StrictLongRunningType is accepted.
| export const DEFAULT_BACKOFF_SETTINGS: StrictLongRunningType = { | |
| export const DEFAULT_BACKOFF_SETTINGS: BackoffSettings = { |
| export function getNextDelay( | ||
| numConsecutiveErrors: number, | ||
| config: BackoffSettings, | ||
| config: StrictLongRunningType, |
There was a problem hiding this comment.
Use the more descriptive BackoffSettings type name if the suggestion to rename StrictLongRunningType is accepted.
| config: StrictLongRunningType, | |
| config: BackoffSettings, |
Description
This pull request adds a yaml file to instruct the Bigtable system test CI check to work with the new Cloud Build trigger thereby making the new CI check effectively run our system tests. It also includes minor code modifications to the Bigtable handwritten layer that allow the system tests to operate properly in the new Cloud Build Environment.
Impact
Leverages the strengths of running system tests in GCB rather than relying on kokoro for system tests.
Testing
This pull request tells the tests how to work with the new check to the continuous integration pipeline for the Bigtable system tests thus improves the effectiveness of that test.
Next Steps