test: run the bigtable system tests in google cloud build#8245
test: run the bigtable system tests in google cloud build#8245danieljbruce 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 adds a Cloud Build configuration for the Bigtable library and refactors tabular-api-surface.ts to use a local type definition, which avoids packaging issues related to internal google-gax imports. It also updates system tests to include necessary Node.js type references. The reviewer suggested several improvements to the build pipeline, including removing redundant global npm updates, using standard binary paths for coverage tools, and replacing hardcoded project IDs with the built-in ${PROJECT_ID} variable for better portability.
| npm install -g npm@latest | ||
| npm config set prefix /usr/local | ||
| npm install | ||
| npm run presystem-test |
There was a problem hiding this comment.
Updating npm globally and changing its prefix during the build process increases execution time and is generally unnecessary in the gcr.io/cloud-builders/npm environment. The builder image already provides a stable version of npm. Removing these steps will streamline the CI pipeline and improve build speed.
npm install
npm run presystem-test| if [ -f ./node_modules/nyc/bin/nyc.js ]; then | ||
| ./node_modules/nyc/bin/nyc.js report || true # `|| true` prevents build failure if nyc report itself exits non-zero |
There was a problem hiding this comment.
Reaching into node_modules/nyc/bin/nyc.js is brittle as the internal structure of the package might change. It is more idiomatic to use the binary located in ./node_modules/.bin/nyc.
if [ -f ./node_modules/.bin/nyc ]; then
./node_modules/.bin/nyc report || true # `|| true` prevents build failure if nyc report itself exits non-zero| # 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 build configuration specific to a single project and less portable. It is recommended to use the built-in ${PROJECT_ID} variable, which automatically resolves to the project ID where the Cloud Build is running.
_GCP_PROJECT_ID: '${PROJECT_ID}'
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