Skip to content
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

Docs Request: Corrections to improve /setup.sh script #1405

Open
SIR-DG opened this issue Dec 16, 2024 · 0 comments · May be fixed by #1408
Open

Docs Request: Corrections to improve /setup.sh script #1405

SIR-DG opened this issue Dec 16, 2024 · 0 comments · May be fixed by #1408
Labels
base web review To be reviewed by the Base web team

Comments

@SIR-DG
Copy link

SIR-DG commented Dec 16, 2024

What is the issue you are encountering with the docs?

-You're using set -eo pipefail, which is good for failing on errors. However, you should also ensure your script handles errors gracefully, such as checking the return status of critical commands and providing informative error messages.

-BUILDKITE_PULL_REQUEST_BASE_BRANCH and BUILDKITE_BRANCH are environment variables that are assumed to be set. If these are not set in the environment, the script will behave unpredictably.

-Variables like $BASE_BRANCH and $BUILDKITE_BRANCH are not quoted. If they contain spaces or special characters, this can lead to errors. Always quote variables unless you're 100% certain they won't have such characters.

-The yarn --immutable command assumes a frozen lockfile. If the lockfile isn't in sync with the dependencies, this will fail. Consider checking for the lockfile's presence before proceeding.

-The git fetch command might fail if there's no remote branch corresponding to BASE_BRANCH. You should handle this case explicitly.

Links to Impacted Docs

https://github.com/base-org/web/blob/master/tools/ci/setup.sh

Describe the solution you'd like to see.

the present script looks mostly fine,but with certain corrections needed,here is my suggestion for an improved script

#!/bin/bash
set -euo pipefail

echo "--- Installing yarn dependencies"

Disable global cache so that we can cache .yarn/cache in Buildkite

yarn config set enableGlobalCache false

Immutable ensures the lockfile is frozen

if [[ -f yarn.lock ]]; then
echo "--- Verifying lockfile integrity"
yarn --immutable || { echo "Lockfile validation failed! Ensure your lockfile is up to date."; exit 1; }
else
echo "Error: yarn.lock file not found. Aborting."
exit 1
fi

Determine the base branch

BASE_BRANCH="${BUILDKITE_PULL_REQUEST_BASE_BRANCH:-$BUILDKITE_BRANCH}"
if [[ -z "$BASE_BRANCH" ]]; then
echo "Error: Unable to determine base branch. Ensure BUILDKITE_BRANCH is set."
exit 1
fi

echo "--- Updating local '$BASE_BRANCH' base branch"

Fetch the base branch from the remote repository

if git fetch -f --no-tags origin "$BASE_BRANCH:$BASE_BRANCH"; then
echo "--- Successfully updated '$BASE_BRANCH'"
else
echo "Error: Failed to fetch base branch '$BASE_BRANCH'. Ensure it exists in the remote repository."
exit 1
fi

echo "--- Building required packages"
if yarn build; then
echo "--- Build completed successfully"
else
echo "Error: Build failed. Check the logs for details."
exit 1
fi

Additional context

below are what i changed in my new script
-Added meaningful error messages to help debug any failures.
-Used ${VAR:-DEFAULT} syntax to provide fallbacks for potentially unset environment variables.
-Enclosed variable references in double quotes to handle spaces or special characters safely.
-Added a check to ensure the yarn.lock file exists before attempting yarn --immutable.
-Added error handling for the git fetch command.

kindly look into this suggestion. thanks!

@crStiv crStiv linked a pull request Dec 17, 2024 that will close this issue
@wbnns wbnns added state: backlog base web review To be reviewed by the Base web team and removed state: backlog labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base web review To be reviewed by the Base web team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants