-
Notifications
You must be signed in to change notification settings - Fork 668
test: run the bigtable system tests in google cloud build #8192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
f14703f
248638f
cdb2ad1
372a03b
7caf8a0
55b0dec
7a3fadd
5f930f9
f3772ea
83f73dd
d7782b2
6be7893
1f71bd1
e6cc411
c836303
1ab42bc
8e6ff1d
051f019
20b11db
a74e61e
0aae31a
af443d1
57bbdad
f6d43fe
8dd7d70
fd18c1a
bfe5155
e51a2e8
11f861b
88a19c7
c6a65a8
83539cc
9f6587c
9916905
3ce9c3f
45420f8
206070f
93476ce
b8599b8
34475c4
9fcae3c
11f52d2
ae907a5
f786ec8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,16 +1,39 @@ | ||
| steps: | ||
| # 0. Check for changes in handwritten/bigtable | ||
| - name: 'gcr.io/cloud-builders/git' | ||
| id: 'check-changes' | ||
| entrypoint: 'bash' | ||
| args: | ||
| - '-c' | ||
| - | | ||
| git fetch origin main --quiet | ||
| # If on a branch, compare with origin/main. If on main, compare with previous commit. | ||
| DIFF_RANGE="origin/main..." | ||
| if [ "$(git rev-parse HEAD)" == "$(git rev-parse origin/main)" ]; then | ||
| DIFF_RANGE="HEAD~1..HEAD" | ||
| fi | ||
| if git diff --quiet "$DIFF_RANGE" -- . ; then | ||
| echo "No changes in handwritten/bigtable. Skipping tests." | ||
| echo "skip" > /workspace/skip_tests | ||
| else | ||
| echo "Changes detected in handwritten/bigtable. Proceeding with tests." | ||
| fi | ||
| dir: 'handwritten/bigtable' | ||
|
|
||
| # 1. Set up Node.js environment | ||
| - name: 'gcr.io/cloud-builders/npm' | ||
| entrypoint: 'bash' | ||
| args: | ||
| - '-c' | ||
| - | | ||
| if [ -f /workspace/skip_tests ]; then exit 0; fi | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not just terminate the workflow early? Do we have to guard every step? I am not sure how to do this, but I suspect this has to be a built in feature?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gemini-code-assist implement this suggestion
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Google Cloud Build, there is no native 'early exit' or 'skip all' command that can terminate the entire build pipeline from a single step. The approach of using a sentinel file (like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll go with gemini's suggestion on this one. I actually tried to remove this step a day ago, but gemini cli combined all these configs into one config which I didn't want so I abandoned this change. I guess it did it because the configs don't support an early exit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we can remove this now that we removed the first step and we are not writing to workspace/skip_tests anymore. |
||
| npm install -g npm@latest | ||
| npm config set prefix /usr/local | ||
| npm install | ||
| npm run presystem-test | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may get executed twice. I think NPM will automatically execute the pre* commands on run? Can you verify?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. That's true. It does so I think we can remove this. |
||
| dir: 'handwritten/bigtable' | ||
| id: 'install-dependencies' | ||
| waitFor: ['check-changes'] | ||
|
|
||
| # 2. Configure environment variables for the tests and run system tests | ||
| # - GOOGLE_APPLICATION_CREDENTIALS: GCB steps run as a service account | ||
|
|
@@ -22,14 +45,19 @@ steps: | |
| # the GCB service account's inherent permissions. | ||
| # - GCLOUD_PROJECT: Can be passed as a build variable. | ||
| - name: 'gcr.io/cloud-builders/npm' | ||
| entrypoint: 'npm' | ||
| args: ['run', 'system-test'] | ||
| entrypoint: 'bash' | ||
| args: | ||
| - '-c' | ||
| - | | ||
| if [ -f /workspace/skip_tests ]; then exit 0; fi | ||
| npm run system-test | ||
| dir: 'handwritten/bigtable' | ||
| env: | ||
| - 'GCLOUD_PROJECT=${_GCP_PROJECT_ID}' # Pass project ID via build variable | ||
| # If you need specific credentials from Secret Manager, uncomment these: | ||
| # - 'GOOGLE_APPLICATION_CREDENTIALS=/secrets/sa-key.json' | ||
| id: 'run-system-tests' | ||
| waitFor: ['install-dependencies'] | ||
| # For Secret Manager, uncomment these (adjust secret name and volume path as needed): | ||
| # secretEnv: ['SA_KEY'] | ||
| # volumes: | ||
|
|
@@ -42,6 +70,7 @@ steps: | |
| args: | ||
| - '-c' | ||
| - | | ||
| if [ -f /workspace/skip_tests ]; then exit 0; fi | ||
| # Check if nyc is installed and run report | ||
| 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 | ||
|
|
@@ -57,6 +86,7 @@ steps: | |
| echo "Codecov reporting (if desired) would be integrated here." | ||
| dir: 'handwritten/bigtable' | ||
| id: 'coverage-report' | ||
| waitFor: ['run-system-tests'] | ||
|
|
||
| # If you use Secret Manager for credentials, uncomment and configure: | ||
| # availableSecrets: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this step at all since GCB can configure when to run based on change detection? When this config enabled it should only run if there are changes in this directory in the first place, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. In which case, I think we can also remove waitFor: ['check-changes'] which was added earlier because of this step