-
Notifications
You must be signed in to change notification settings - Fork 512
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
Modified the script to be compatible with new K8S version #1050
base: master
Are you sure you want to change the base?
Conversation
/gcbrun |
/gcbrun |
/gcbrun |
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.
This seems fine. Feel free to respond to my questions or comments in the comments after you merge.
@@ -30,6 +39,7 @@ while ! kubectl describe "pod/${POD_NAME}" | grep -q Terminated; do | |||
LOGS_SINCE_TIME=$(date --iso-8601=seconds) | |||
done | |||
|
|||
|
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.
this new newline is probably not necessary
--env="COMMIT_SHA=${COMMIT_SHA}" \ | ||
--env="IMAGE_VERSION=${DATAPROC_IMAGE_VERSION}" \ | ||
--command -- bash /init-actions/cloudbuild/presubmit.sh | ||
#kubectl run "${POD_NAME}" \ |
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.
We should keep this old command in the previous commit in the main branch. Thank you for preserving the previous implementation in comments. I loathe the concept of removing the old implementation as well. We could leave this comment in for the next release for posterity and then remove them to reduce file size and execution time.
- 'COMMIT_SHA=$COMMIT_SHA' | ||
- 'CLOUDSDK_COMPUTE_REGION=us-central1' | ||
- 'CLOUDSDK_CONTAINER_CLUSTER=init-actions-presubmit' | ||
# # Run presubmit tests in parallel for 2.0 Rocky Linux image |
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.
again, leave the comments for posterity for one release and then remove them to reduce file size and execution time
- 'CLOUDSDK_COMPUTE_REGION=us-central1' | ||
- 'CLOUDSDK_CONTAINER_CLUSTER=init-actions-presubmit' | ||
|
||
# # Run presubmit tests in parallel for 1.5 Debian image |
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.
same as the previous/next two comments. leave comments for one PR and remove them in the next to reduce file size and execution time.
@@ -11,6 +11,6 @@ COPY --chown=ia-tests:ia-tests . /init-actions | |||
# https://docs.bazel.build/versions/master/install-ubuntu.html | |||
RUN echo "deb [arch=amd64] http://storage.googleapis.com/bazel-apt stable jdk1.8" | tee /etc/apt/sources.list.d/bazel.list | |||
RUN curl https://bazel.build/bazel-release.pub.gpg | apt-key add - | |||
RUN apt-get update && apt-get install -y openjdk-8-jdk python3-setuptools bazel | |||
RUN apt-get update && apt-get install -y openjdk-8-jdk python3-setuptools bazel gettext-base |
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 seems fine. I don't see it being used below, but perhaps the kubectl apply depends on this for translation. Can you share why we included this package?
No description provided.