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

ci: ensure variables are set for cache build #2948

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

gartnera
Copy link
Member

@gartnera gartnera commented Oct 1, 2024

#2928 introduced these logs in E2E CI:

#19 [latest-build 6/6] RUN --mount=type=cache,target="/root/.cache/go-build"     NODE_VERSION=${NODE_VERSION}     NODE_COMMIT=${NODE_COMMIT}     make install install-zetae2e
#19 0.187 fatal: not a git repository (or any of the parent directories): .git
#19 0.188 fatal: not a git repository (or any of the parent directories): .git
#19 0.189 warning: Not a git repository. Use --no-index to compare two paths outside a working tree
#19 0.189 usage: git diff --no-index [<options>] <path> <path>
#19 0.189 
#19 0.189 Diff output format options
#19 0.189     -p, --patch           generate patch
#19 0.189     -s, --no-patch        suppress diff output
#19 0.189     -u                    generate patch
#19 0.189     -U, --unified[=<n>]   generate diffs with <n> lines context
#19 0.189     -W, --function-context
#19 0.189                           generate diffs with <n> lines context
#19 0.189     --raw                 generate the diff in raw format
#19 0.189     --patch-with-raw      synonym for '-p --raw'
#19 0.189     --patch-with-stat     synonym for '-p --stat'
#19 0.189     --numstat             machine friendly --stat
#19 0.190     --shortstat           output only the last line of --stat
<snip>

This is because the cache build also needs to have the version set. But considering it passed all CI it might make sense to just keep the version static in e2e so that build cache performance is improved.

Summary by CodeRabbit

  • New Features

    • Introduced new environment variables NODE_VERSION and NODE_COMMIT to enhance the caching mechanism during Docker builds.
  • Improvements

    • Updated the build process to ensure consistency between cache builds and make builds by utilizing the new environment variables.

@gartnera gartnera added no-changelog Skip changelog CI check UPGRADE_LIGHT_TESTS Run make start-upgrade-test-light labels Oct 1, 2024
Copy link
Contributor

coderabbitai bot commented Oct 1, 2024

📝 Walkthrough

Walkthrough

The changes implemented in the pull request enhance the GitHub Actions workflow by introducing new environment variables for the Node.js version and the latest commit hash. A new step, "Set version for cache," has been added to retrieve and set these variables, ensuring consistency during the Docker image build process. Additionally, the Docker build step has been updated to utilize these new variables, improving the caching mechanism while maintaining the overall structure of the workflow.

Changes

File Change Summary
.github/workflows/reusable-e2e.yml Added a new step to set NODE_VERSION and NODE_COMMIT environment variables for Docker build. Updated the Docker build step to include these variables as build arguments.

Possibly related PRs

  • chore: improve localnet build performance #2928: The changes in this PR introduce new arguments NODE_VERSION and NODE_COMMIT in the Dockerfile, which directly relate to the new environment variables set in the main PR for consistent versioning during the Docker build process.

Suggested reviewers

  • morde08
  • skosito
  • lumtis
  • kingpinXD
  • fbac

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between f9cbcfc and 2aa7099.

📒 Files selected for processing (1)
  • .github/workflows/reusable-e2e.yml (2 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/reusable-e2e.yml

74-74: shellcheck reported issue in this script: SC2086:info:2:38: Double quote to prevent globbing and word splitting

(shellcheck)


74-74: shellcheck reported issue in this script: SC2086:info:4:36: Double quote to prevent globbing and word splitting

(shellcheck)

🔇 Additional comments (3)
.github/workflows/reusable-e2e.yml (3)

Line range hint 72-96: Summary: Effective implementation with minor suggestions for improvement.

The changes introduced in this PR effectively address the issue of setting necessary variables for the cache build. The addition of the "Set version for cache" step and the incorporation of build arguments in the Docker build step align well with the PR objectives.

Key points:

  1. The new step ensures consistency between the cache build and the make build.
  2. The Docker build step now includes the version information, addressing the issue mentioned in the PR summary.

The suggested improvements and verifications will further enhance the robustness and safety of the implementation. Overall, these changes should resolve the CI errors related to Git repository detection and improve the E2E CI process.

🧰 Tools
🪛 actionlint

74-74: shellcheck reported issue in this script: SC2086:info:2:38: Double quote to prevent globbing and word splitting

(shellcheck)


74-74: shellcheck reported issue in this script: SC2086:info:4:36: Double quote to prevent globbing and word splitting

(shellcheck)


94-96: Approve addition of build arguments with verification suggestion.

The addition of NODE_VERSION and NODE_COMMIT as build arguments in the Docker build step is appropriate and aligns with the PR objective. This ensures that the Docker image is built with the correct version information, addressing the issue mentioned in the PR summary.

To verify the correct implementation:

This script will help ensure that the Dockerfile is prepared to receive and use these build arguments.

✅ Verification successful

Docker Build Arguments Verified Successfully

The NODE_VERSION and NODE_COMMIT build arguments are correctly implemented in Dockerfile-localnet at lines 22 and 23. This ensures that the Docker image is built with the appropriate version information as intended.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that the Dockerfile-localnet uses the NODE_VERSION and NODE_COMMIT build args
grep -n "ARG NODE_VERSION" Dockerfile-localnet
grep -n "ARG NODE_COMMIT" Dockerfile-localnet

Length of output: 135


72-79: Approve addition of version setting step with suggestions for improvement.

The new step "Set version for cache" effectively addresses the PR objective by setting the necessary NODE_VERSION and NODE_COMMIT variables. This ensures consistency between the cache build and the make build. However, there are a few points to consider for improvement:

  1. To enhance shell script safety, consider using double quotes around variable expansions:

    run: |
      NODE_VERSION="$(./version.sh)"
      echo "NODE_VERSION=${NODE_VERSION}" >> $GITHUB_ENV
      NODE_COMMIT="$(git log -1 --format='%H')"
      echo "NODE_COMMIT=${NODE_COMMIT}" >> $GITHUB_ENV
  2. It's assumed that version.sh exists in the repository root. To ensure robustness:

✅ Verification successful

Approve addition of version setting step with improvements for shell script safety.

The new step "Set version for cache" effectively addresses the PR objective by setting the necessary NODE_VERSION and NODE_COMMIT variables. The existence of version.sh in the repository root has been confirmed, ensuring robustness.

However, there are a few points to consider for improvement:

  1. To enhance shell script safety, consider using double quotes around variable expansions:
    run: |
      NODE_VERSION="$(./version.sh)"
      echo "NODE_VERSION=${NODE_VERSION}" >> $GITHUB_ENV
      NODE_COMMIT="$(git log -1 --format='%H')"
      echo "NODE_COMMIT=${NODE_COMMIT}" >> $GITHUB_ENV
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the existence and location of version.sh
if [ -f "./version.sh" ]; then
  echo "version.sh found in the repository root."
else
  echo "version.sh not found in the repository root. Searching for it..."
  fd -t f "version.sh"
fi

Length of output: 116

🧰 Tools
🪛 actionlint

74-74: shellcheck reported issue in this script: SC2086:info:2:38: Double quote to prevent globbing and word splitting

(shellcheck)


74-74: shellcheck reported issue in this script: SC2086:info:4:36: Double quote to prevent globbing and word splitting

(shellcheck)


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the ci Changes to CI pipeline or github actions label Oct 1, 2024
@gartnera gartnera marked this pull request as ready for review October 1, 2024 18:31
@gartnera gartnera requested a review from a team as a code owner October 1, 2024 18:31
@gartnera gartnera added this pull request to the merge queue Oct 2, 2024
Merged via the queue into develop with commit fce09b2 Oct 2, 2024
39 checks passed
@gartnera gartnera deleted the e2e-cache-build-version branch October 2, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Changes to CI pipeline or github actions no-changelog Skip changelog CI check UPGRADE_LIGHT_TESTS Run make start-upgrade-test-light
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants