-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(infra): Add script to set up SPM-based quickstarts #15295
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
base: main
Are you sure you want to change the base?
Conversation
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) 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. |
/gemini review |
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.
Code Review
This pull request introduces a new script, scripts/setup_quickstart_spm.sh
, to automate the setup of SPM-based quickstart projects for integration testing. The script is well-structured, using set -euo pipefail
for robustness and providing clear usage instructions. My review identifies a few areas for improvement. The most critical issue is a hardcoded git checkout
to a specific branch, which should be removed to make the script generally usable. Other suggestions include minor shell scripting best practices, such as consistently using [[ ... ]]
for tests and quoting variables and paths to enhance script reliability.
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.
Code Review
This pull request introduces a new script, setup_quickstart_spm.sh
, to automate the setup of SPM-based quickstarts for integration testing. The script is well-structured, with clear argument parsing and different modes for various testing scenarios. My review focuses on improving the script's robustness and maintainability by addressing a hardcoded branch name, suggesting the use of more modern bash syntax, and ensuring variables are properly quoted.
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.
LGTM. Most of the Gemini suggestions also seem reasonable to me apart from changing mc/spm
, which I think was intentional (but please double check my understanding).
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
/gemini review |
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.
Code Review
This pull request introduces a new script, setup_quickstart_spm.sh
, to automate the setup of SPM-based quickstarts for integration testing. The script is well-structured and covers different testing scenarios. My review focuses on improving its robustness and maintainability. I've suggested changes to more reliably locate the Xcode project file, ensure file paths are correctly resolved to absolute paths, and enhance the readability of the logic for selecting testing modes.
/gemini review |
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.
Code Review
This pull request introduces a new bash script scripts/setup_quickstart_spm.sh
to automate the setup of SPM-based quickstart projects for integration testing. The script is well-structured, uses shell best practices like set -euo pipefail
, and handles different testing scenarios (PR, nightly, prerelease) correctly. It also includes logic for both local runs (with a local quickstart repo) and CI runs (cloning the repo). My review includes a few minor suggestions to improve the robustness and maintainability of the script.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
/gemini review |
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.
Code Review
This pull request introduces a new bash script scripts/setup_quickstart_spm.sh
to automate the setup of SPM-based quickstart projects for integration testing. The script is well-structured, uses shell best practices like set -euo pipefail
, and handles different testing scenarios (PR, nightly, prerelease) correctly. It also includes logic for both local runs (with a local quickstart repo) and CI runs (cloning the repo). My review includes a few suggestions to improve the robustness and maintainability of the script, including fixing a bug related to an unused environment variable and simplifying some logic.
|
||
# Some quickstarts may not need a real GoogleService-Info.plist for their tests. | ||
# When QUICKSTART_REPO is set, we are running locally and should skip the secrets check. | ||
if [[ -n "${QUICKSTART_REPO:-}" ]] || check_secrets || [[ "${SAMPLE}" == "installations" ]]; then |
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.
The script's documentation (lines 44-45) mentions the GHA_WORKFLOW_SECRET
environment variable to bypass the secret check for local runs, but the variable is not used in the logic here. This means local runs that need to clone the quickstart repo (i.e., when QUICKSTART_REPO
is not set) will incorrectly fail the secret check. The check for this variable should be added to this condition.
if [[ -n "${QUICKSTART_REPO:-}" ]] || check_secrets || [[ "${SAMPLE}" == "installations" ]]; then | |
if [[ -n "${QUICKSTART_REPO:-}" ]] || [[ "${GHA_WORKFLOW_SECRET:-}" == "true" ]] || check_secrets || [[ "${SAMPLE}" == "installations" ]]; then |
root_dir="$(dirname "$scripts_dir")" | ||
|
||
# Source function to check if CI secrets are available. | ||
source "$scripts_dir/check_secrets.sh" |
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.
Sourcing check_secrets.sh
here will cause shell tracing (set -x
) to be enabled for the remainder of this script's execution, as check_secrets.sh
contains a set -x
command. This overrides the intended behavior of only enabling tracing when DEBUG=true
and can lead to excessively verbose logs in CI. Consider removing set -x
from check_secrets.sh
in a separate change to avoid this side effect.
if [[ -z "${QUICKSTART_REPO:-}" ]]; then | ||
# When cloning, the project file path is relative to the SDK root. | ||
ABSOLUTE_PROJECT_FILE="$root_dir/$PROJECT_FILE" | ||
else | ||
# When using a local repo, the path could be relative to the current | ||
# directory. Resolve it to an absolute path. | ||
ABSOLUTE_PROJECT_FILE="$(cd "$(dirname "$PROJECT_FILE")" && pwd)/$(basename "$PROJECT_FILE")" | ||
fi |
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.
The logic to resolve the absolute path of the project file can be simplified. The method used in the else
block is robust enough to handle both the local repo case (when QUICKSTART_REPO
is set) and the CI case (when it's not). Using a single, consistent method improves code readability and maintainability.
if [[ -z "${QUICKSTART_REPO:-}" ]]; then | |
# When cloning, the project file path is relative to the SDK root. | |
ABSOLUTE_PROJECT_FILE="$root_dir/$PROJECT_FILE" | |
else | |
# When using a local repo, the path could be relative to the current | |
# directory. Resolve it to an absolute path. | |
ABSOLUTE_PROJECT_FILE="$(cd "$(dirname "$PROJECT_FILE")" && pwd)/$(basename "$PROJECT_FILE")" | |
fi | |
# The update script needs an absolute path to the project file. | |
ABSOLUTE_PROJECT_FILE="$(cd "$(dirname "$PROJECT_FILE")" && pwd)/$(basename "$PROJECT_FILE")" |
This pull request introduces a new bash script scripts/setup_quickstart_spm.sh to automate the setup of SPM-based quickstart projects for integration testing. The script is well-structured, uses shell best practices like set -euo pipefail, and handles different testing scenarios (PR, nightly, prerelease) correctly. It also includes logic for both local runs (with a local quickstart repo) and CI runs (cloning the repo). My review includes a few minor suggestions to improve the robustness and maintainability of the script.
#no-changelog