feat: add new Bigtable feature#8192
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates system tests from Kokoro to Google Cloud Build for the handwritten/bigtable project. The existing Kokoro script is replaced with a notification, and a new cloudbuild.yaml is introduced. Feedback focuses on correcting the Cloud Build configuration, specifically addressing issues with environment variable persistence across steps, incorrect working directory paths, and the execution order of pre-test hooks. Consolidating steps and ensuring the correct directory context is used will prevent build failures and maintain coverage reporting.
I am having trouble creating individual review comments. Click here to see my feedback.
handwritten/bigtable/cloudbuild.yaml (6-55)
The current multi-step configuration has several issues that will likely cause the build to fail or behave incorrectly:
- Environment Variable Persistence: Each Cloud Build step runs in a separate container. Environment variables exported by
.kokoro/pre-system-test.shin Step 2 will not be available to Step 3. - Working Directory: Cloud Build's default working directory is the repository root. Since this project is located in
handwritten/bigtable, you must specifydir: 'handwritten/bigtable'for the steps to findpackage.jsonand the.kokorodirectory. The currentdir: '.'refers to the repo root, not the directory containing thecloudbuild.yamlfile. - Execution Order: The original Kokoro script ran the pre-test hook before
npm install. This is important if the hook configures registry credentials or environment-specific settings needed for installation. - Efficiency:
npm install -g npm@latestand global prefix configuration are generally unnecessary in the Cloud Build environment and add overhead.
Consolidating these into a single step with the correct dir and execution order ensures correctness and improves performance.
- name: 'gcr.io/cloud-builders/npm'
id: 'run-system-tests'
entrypoint: 'bash'
dir: 'handwritten/bigtable'
args:
- '-c'
- |
if [ -f .kokoro/pre-system-test.sh ]; then
echo "Running pre-system-test.sh..."
. .kokoro/pre-system-test.sh
fi
npm install
npm run system-test
env:
- 'GCLOUD_PROJECT=${_GCP_PROJECT_ID}'handwritten/bigtable/cloudbuild.yaml (58-77)
This step also requires the correct working directory (dir: 'handwritten/bigtable') to find the coverage artifacts. Additionally, the original Kokoro script uploaded coverage via codecov.sh, which is currently missing here. Consider adding the logic to upload reports to Codecov to avoid a regression in coverage tracking.
- name: 'gcr.io/cloud-builders/npm'
id: 'coverage-report'
entrypoint: 'bash'
dir: 'handwritten/bigtable'
args:
- '-c'
- |
if [ -f ./node_modules/nyc/bin/nyc.js ]; then
./node_modules/nyc/bin/nyc.js report || true
else
echo "nyc not found, skipping coverage report."
fi
# TODO: Implement codecov upload logic here.
echo "Codecov reporting (if desired) would be integrated here."…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
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕