chore: Migrate Skaffold presubmits to new Kokoro instance#9952
chore: Migrate Skaffold presubmits to new Kokoro instance#9952menahyouyeah wants to merge 18 commits intomainfrom
Conversation
Summary of ChangesHello @menahyouyeah, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request orchestrates a significant infrastructure update for Skaffold's presubmit checks. The core objective is to transition these checks to a new, dedicated Kokoro environment and GCP project, leveraging Google Artifact Registry for image storage. This change enhances the build and test pipeline's scalability and organization by separating presubmit workflows from the main CI/Release infrastructure, ensuring that development iterations benefit from the new setup without disrupting existing production pipelines. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request migrates the Skaffold presubmits to a new Kokoro instance and GCP project, introducing conditional logic in the Makefile to switch between GCR and Artifact Registry based on the GCP_PROJECT variable. The changes are logical and follow the PR's goal.
However, I've found a few critical issues that would cause the build to fail or behave incorrectly:
- The
Makefilehas a logic error where a variable (DEPS_DIGEST) is used without being defined in theskaffold-builder-citarget. - Another
docker buildcommand is broken due to a missing line continuation character and inconsistent indentation. - The presubmit script (
hack/kokoro/presubmit.sh) is missing required environment variables to trigger the new build logic in theMakefile.
I've also included a high-severity suggestion to make the Makefile more robust by adding checks for required variables, and a medium-severity comment for a typo. Please address these issues to ensure the new presubmit pipeline works as expected.
| -t gcr.io/$(GCP_PROJECT)/skaffold:$(VERSION) \ | ||
| -t gcr.io/$(GCP_PROJECT)/skaffold:latest \ | ||
| -t $(IMAGE_REPO_BASE)/skaffold:$(VERSION) \ | ||
| -t $(IMAGE_REPO_BASE)/skaffold:latest \ |
There was a problem hiding this comment.
This line has two issues:
- It uses spaces for indentation, while the surrounding lines and Makefile conventions use tabs. This is inconsistent.
- It is missing a trailing
\to continue thedocker buildcommand on the next line. This will cause the build to fail.
Please fix the indentation and add the line continuation character.
-t $(IMAGE_REPO_BASE)/skaffold:latest \
| docker push $(IMAGE_REPO_BASE)/build_deps:$(DEPS_DIGEST) | ||
|
|
||
|
|
||
| skaffold-builder-ci: |
There was a problem hiding this comment.
The skaffold-builder-ci target uses the DEPS_DIGEST variable in a --cache-from flag, but DEPS_DIGEST is not defined within this target's scope. This will result in DEPS_DIGEST being empty, causing the cache to be missed or pointing to an incorrect image tag. You should define DEPS_DIGEST within the skaffold-builder-ci target.
skaffold-builder-ci:
$(eval DEPS_DIGEST := $(shell ./hack/skaffold-deps-sha1.sh))
hack/kokoro/presubmit.sh
Outdated
|
|
||
| # hanges the current directory to where Kokoro has checked out the GitHub repository. | ||
| pushd $KOKORO_ARTIFACTS_DIR/github/skaffold >/dev/null | ||
| GCP_ONLY=true make integration-in-docker |
There was a problem hiding this comment.
This make command is missing essential environment variables (GCP_PROJECT, AR_REGION, GKE_REGION) required to trigger the new presubmit logic in the Makefile.
GCP_PROJECTwill default tok8s-skaffold, and theskaffold-ci-cdspecific logic will not be used.AR_REGIONandGKE_REGIONwill be unset, causing errors or incorrect behavior in the Makefile.
Please provide these variables to the make command to ensure the new presubmit infrastructure is used.
| GCP_ONLY=true make integration-in-docker | |
| GCP_ONLY=true GCP_PROJECT=skaffold-ci-cd AR_REGION=us-central1 GKE_REGION=us-central1 make integration-in-docker |
| ifeq ($(GCP_PROJECT),skaffold-ci-cd) | ||
| # Presubmit environment: skaffold-ci-cd project with Artifact Registry |
There was a problem hiding this comment.
The new logic for GCP_PROJECT=skaffold-ci-cd relies on AR_REGION and GKE_REGION variables. However, these variables are not defined with default values, nor are there any checks to ensure they are set. If they are not provided in the environment, they will be empty, leading to malformed values for IMAGE_REPO_BASE and GKE_LOCATION_FLAG, which can cause silent failures or hard-to-debug issues. It's safer to add checks to ensure these required variables are set.
ifeq ($(GCP_PROJECT),skaffold-ci-cd)
ifndef AR_REGION
$(error AR_REGION must be set when GCP_PROJECT is skaffold-ci-cd)
endif
ifndef GKE_REGION
$(error GKE_REGION must be set when GCP_PROJECT is skaffold-ci-cd)
endif
# Presubmit environment: skaffold-ci-cd project with Artifact Registry
hack/kokoro/presubmit.sh
Outdated
| # Any docker push commands will push images here | ||
| export DOCKER_NAMESPACE=us-central1-docker.pkg.dev/skaffold-ci-cd/skaffold-images | ||
|
|
||
| # hanges the current directory to where Kokoro has checked out the GitHub repository. |
e37297b to
2e93229
Compare
Description
This PR updates the Skaffold build and test infrastructure to run presubmit checks on the new dedicated Kokoro instance (
skaffold-presubmit) and GCP project (skaffold-ci-cd).Key changes:
skaffold-ci-cdfor presubmits, while retaining GCR for CI/Release ink8s-skaffold.CI and Release processes remain on the existing infrastructure for now.