fix: adds config file and shell script to run storage cloudbuild tests#14022
fix: adds config file and shell script to run storage cloudbuild tests#14022chalmerlowe wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new CI/CD pipeline for zonal system tests, including a Cloud Build configuration and a helper shell script to run tests on a GCE VM. Feedback focuses on correcting the repository URL and directory paths to align with the current monorepo structure, as well as addressing potential issues with aggressive SSH key cleanup that could interfere with concurrent builds. Other recommendations include adding missing substitutions for better documentation, using a stable Debian image, simplifying service account scopes, and adding a shebang to the shell script.
| cd python-docs-samples | ||
| git sparse-checkout set main/storage | ||
| git fetch origin "refs/pull/${_PR_NUMBER}/head" | ||
| git checkout ${COMMIT_SHA} | ||
| cd main/storage |
There was a problem hiding this comment.
The directory paths include a main/ prefix (e.g., git sparse-checkout set main/storage, cd main/storage) which does not match the standard repository structure. In the monorepo, the storage component is located at the root level storage/.
| cd python-docs-samples | |
| git sparse-checkout set main/storage | |
| git fetch origin "refs/pull/${_PR_NUMBER}/head" | |
| git checkout ${COMMIT_SHA} | |
| cd main/storage | |
| cd google-cloud-python | |
| git sparse-checkout set storage | |
| git fetch origin "refs/pull/${_PR_NUMBER}/head" | |
| git checkout ${COMMIT_SHA} | |
| cd storage |
| substitutions: | ||
| _REGION: "us-central1" | ||
| _ZONE: "us-central1-a" | ||
| _SHORT_BUILD_ID: ${BUILD_ID:0:8} | ||
| _VM_NAME: "py-sdk-sys-test-${_SHORT_BUILD_ID}" | ||
| _ULIMIT: "10000" # 10k, for gRPC bidi streams |
There was a problem hiding this comment.
Several variables used in the build steps (such as _ZONAL_VM_SERVICE_ACCOUNT, _ZONAL_BUCKET, and _PR_NUMBER) are missing from the substitutions block. Adding them with default values improves readability and ensures the template is self-documenting.
substitutions:
_REGION: "us-central1"
_ZONE: "us-central1-a"
_SHORT_BUILD_ID: ${BUILD_ID:0:8}
_VM_NAME: "py-sdk-sys-test-${_SHORT_BUILD_ID}"
_ULIMIT: "10000" # 10k, for gRPC bidi streams
_ZONAL_VM_SERVICE_ACCOUNT: ""
_ZONAL_BUCKET: ""
_CROSS_REGION_BUCKET: ""
_PR_NUMBER: ""| id: "cleanup-old-keys" | ||
| entrypoint: "bash" | ||
| args: | ||
| - "-c" | ||
| - | | ||
| #!/bin/bash | ||
| set -e | ||
|
|
||
| echo "Fetching OS Login SSH keys..." | ||
| echo "Removing all keys." | ||
| echo "---------------------------------------------------------------------" | ||
|
|
||
| FINGERPRINTS_TO_DELETE=$$(gcloud compute os-login ssh-keys list \ | ||
| --format="value(fingerprint)") | ||
|
|
||
| echo "Keys to delete: $$FINGERPRINTS_TO_DELETE" | ||
|
|
||
| if [ -z "$$FINGERPRINTS_TO_DELETE" ]; then | ||
| echo "No keys found to delete. Nothing to do." | ||
| exit 0 | ||
| fi | ||
|
|
||
| while IFS= read -r FINGERPRINT; do | ||
| if [ -n "$$FINGERPRINT" ]; then | ||
| echo "Deleting key with fingerprint: $$FINGERPRINT" | ||
| gcloud compute os-login ssh-keys remove \ | ||
| --key="$$FINGERPRINT" \ | ||
| --quiet || true | ||
| fi | ||
| done <<< "$$FINGERPRINTS_TO_DELETE" | ||
|
|
||
| echo "---------------------------------------------------------------------" | ||
| echo "Cleanup complete." |
There was a problem hiding this comment.
The cleanup-old-keys step is extremely aggressive as it deletes all OS Login SSH keys associated with the service account. This can cause intermittent failures for concurrent builds using the same service account in the same project. It is safer to rely on the specific cleanup-ssh-key step at the end of the build.
| sleep 15 | ||
| done | ||
| # copy the script to the VM | ||
| gcloud compute scp main/storage/cloudbuild/run_zonal_tests.sh ${_VM_NAME}:~ --zone=${_ZONE} --internal-ip --ssh-key-file=/workspace/.ssh/google_compute_engine |
There was a problem hiding this comment.
The scp path includes a main/ prefix which likely does not exist in the Cloud Build workspace. The path should be relative to the repository root.
gcloud compute scp storage/cloudbuild/run_zonal_tests.sh ${_VM_NAME}:~ --zone=${_ZONE} --internal-ip --ssh-key-file=/workspace/.ssh/google_compute_engine|
|
||
| set -euxo pipefail |
There was a problem hiding this comment.
The shell script is missing a shebang line. It is recommended to start with #!/bin/bash to ensure it is executed with the correct interpreter.
| set -euxo pipefail | |
| #!/bin/bash | |
| set -euxo pipefail |
| - "--project=${PROJECT_ID}" | ||
| - "--zone=${_ZONE}" | ||
| - "--machine-type=e2-medium" | ||
| - "--image-family=debian-13" |
There was a problem hiding this comment.
Debian 13 is currently the 'testing' distribution. It is generally safer to use the current stable version, debian-12, for CI/CD environments to ensure stability and availability of packages.
- "--image-family=debian-12"| - "--image-family=debian-13" | ||
| - "--image-project=debian-cloud" | ||
| - "--service-account=${_ZONAL_VM_SERVICE_ACCOUNT}" | ||
| - "--scopes=https://www.googleapis.com/auth/devstorage.full_control,https://www.googleapis.com/auth/devstorage.read_only,https://www.googleapis.com/auth/devstorage.read_write" |
There was a problem hiding this comment.
The scopes provided are redundant. https://www.googleapis.com/auth/devstorage.full_control already includes read and write permissions. Using https://www.googleapis.com/auth/cloud-platform is the recommended best practice when using IAM for access control.
- "--scopes=https://www.googleapis.com/auth/cloud-platform"
Description
Partially Fixes #googleapis/google-cloud-python#16487
This PR is offered as starting point to enable cloudbuild tests for the Storage Team.
This mirrors a config and script that was in use in the split repo prior to migrating
python-storageinto thegoogle-cloud-pythonmonorepo.I am not versed in all the nuances of how this needs to be configured. @chandra-siri and @chingor13 were instrumental in getting similar tests running in
google-cloud-python.I think these task apply, but as I said: not the expert.