Optimize build and release pipelines #557
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
January 30, 2024 14:03
fix python scripts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .github/workflows/build.yml (1 hunks)
- build/docker/Dockerfile (1 hunks)
- tests/Dockerfile (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- build/docker/Dockerfile
- tests/Dockerfile
Additional comments: 31
.github/workflows/build.yml (31)
- 5-6: The addition of
pull_request_reviewevents with typesubmittedis a good practice for triggering workflows on review submissions. Ensure that downstream jobs or steps that depend on pull request reviews being approved are correctly gated.- 13-13: The conditional execution of the build job based on
workflow_dispatchor approved pull requests targetingmasteris correctly implemented. This ensures that builds are only triggered for relevant events and approved changes.- 16-20: Using a local registry service for Docker image caching and testing is a good practice. It can improve build times and reduce dependency on external services. Ensure the registry is secured and not exposed unnecessarily.
- 28-34: The checkout step with
set-safe-directoryandsubmodules: recursiveensures that the repository and its submodules are correctly checked out, which is crucial for a complete build environment.- 36-41: Caching GraphBLAS based on the platform and hash of
GraphBLAS.his efficient for build performance. However, ensure that the cache paths and keys are correctly configured to match the actual build outputs and dependencies.- 43-48: Similar to GraphBLAS caching, caching the parser library is well-implemented. It's important to verify that the cache is effectively utilized in subsequent steps to avoid redundant builds.
- 50-55: Caching the search library follows the same pattern as previous caches. Consistency in caching strategy across different dependencies is good for maintainability.
- 59-62: Relocating the source to a consistent working directory is a good practice for ensuring that build paths are predictable and stable across different runs.
- 64-71: Setting up QEMU and Docker Buildx with
network=hostdriver-opt is necessary for cross-platform builds and pushing to a local registry. This configuration supports building images for multiple architectures.- 73-84: Building the compiler image with Docker Buildx and pushing it to a local registry is correctly implemented. Ensure that the build context, Dockerfile path, and build arguments are correctly specified for the intended build environment.
- 86-93: Copying binaries from the Docker compiler image for caching is a smart way to reuse build artifacts. Ensure that the conditional check for cache hits is correctly implemented to avoid unnecessary steps.
- 95-106: Building the base image with the correct context, Dockerfile, and build arguments, including the use of the compiler image as the base, is well-implemented. This step is crucial for creating a consistent and reproducible build environment.
- 108-113: Uploading the built image as an artifact is a good practice for sharing build outputs between jobs. Ensure that the artifact name and path match the expected outputs of the build step.
- 115-119: Exporting the compiler image for tests only on the
linux/amd64platform might limit testing on other architectures. Consider if this step should be conditional or consistent across all supported platforms.- 121-126: Uploading the compiler image as an artifact for tests is correctly implemented. This step is necessary for making the compiler image available to subsequent testing jobs.
- 132-136: Reusing the registry service setup in the merge job is consistent with the build job. This consistency is good for maintainability and understanding the workflow.
- 138-144: Repeating the safe directory configuration and checkout step in the merge job ensures that the repository is correctly prepared for merging operations.
- 146-150: Downloading the compiler image artifact in the merge job is necessary for building and pushing the final images. Ensure that the artifact name matches the one uploaded in the build job.
- 158-162: Loading the compiler image in the merge job is correctly implemented. This step is crucial for using the compiler image in subsequent image building and tagging operations.
- 163-166: Tagging the compiler image for use in the merge job is a necessary step for consistency in image references across the workflow.
- 167-169: Listing Docker images as a diagnostic step can help with debugging the workflow but consider if this step is necessary for the final workflow or if it can be removed to streamline the process.
- 171-173: Pushing the compiler image to the local registry in the merge job is correctly implemented. This step ensures that the image is available for subsequent steps and jobs.
- 175-177: Pulling the compiler image from the local registry after pushing it seems redundant. This step could potentially be removed to streamline the workflow unless there's a specific reason for its inclusion.
- 179-189: Building the test image with the correct base image and target platform is well-implemented. This step is crucial for preparing the environment for running tests.
- 191-195: Uploading the test image as an artifact is correctly implemented. This step is necessary for making the test image available to subsequent testing jobs.
- 202-206: Downloading the test image artifact in each testing job (unit-tests, flow-tests, tck-tests, fuzz-tests) is correctly implemented. This step ensures that the correct test environment is used.
- 208-211: Loading the test image in each testing job is a necessary step for preparing the Docker environment for test execution.
- 214-215: Executing unit tests using the test image is correctly implemented. This step is crucial for validating the correctness of the code changes.
- 221-234: The steps for downloading, loading, and running flow tests are correctly implemented, following the same pattern as unit tests. This consistency is good for maintainability.
- 240-253: The steps for downloading, loading, and running TCK tests are correctly implemented, ensuring that the test environment is consistent across all test types.
- 259-272: The steps for downloading, loading, and running fuzz tests are correctly implemented. The inclusion of a timeout for fuzz tests is a good practice for preventing tests from running indefinitely.
added 2 commits
February 4, 2024 14:42
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- .github/workflows/build.yml (1 hunks)
- build/docker/Dockerfile (1 hunks)
- build/docker/Dockerfile.compiler (1 hunks)
- tests/Dockerfile (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- build/docker/Dockerfile
- build/docker/Dockerfile.compiler
Additional comments: 13
tests/Dockerfile (2)
- 24-24: Ensure that the
testsdirectory exists and contains the expected content before copying it into the Docker image.- 27-27: Verify that
/FalkorDB/tests/requirements.txtincludes all necessary dependencies for comprehensive testing..github/workflows/build.yml (11)
- 5-9: Ensure that the
pull_request_reviewtrigger is appropriate for the project's workflow and that it doesn't lead to unnecessary builds.- 13-13: The condition for the build job might be too restrictive. Verify that it aligns with the project's requirements for when builds should be triggered.
- 16-20: Using a local registry service is a good practice for testing image builds and pushes. Ensure the registry service is properly secured and configured.
- 36-55: Caching dependencies is a good practice. Ensure that the paths and keys for caches are correctly set up to maximize cache hits.
- 64-71: Verify that the
network=hostdriver option in the Docker Buildx setup does not introduce any security or performance issues.- 73-85: Ensure that the compiler image is built with the correct context and that the
TARGETPLATFORMargument is properly utilized.- 95-105: Verify that the test image is built with the correct base image and that the
TARGETPLATFORMargument is properly utilized.- 135-155: Ensure that the unit tests are correctly configured to run using the downloaded and loaded test image.
- 159-179: Verify that the flow tests are correctly configured to run using the downloaded and loaded test image.
- 183-203: Ensure that the TCK tests are correctly configured to run using the downloaded and loaded test image.
- 207-227: Verify that the fuzz tests are correctly configured to run using the downloaded and loaded test image and that the
TIMEOUTparameter is appropriately set.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/build.yml (1 hunks)
- tests/Dockerfile (1 hunks)
Additional comments: 21
tests/Dockerfile (3)
- 7-7: Use
--no-install-recommendswithapt-get installto minimize the image size.- 13-13: Use
apt-get install -y --no-install-recommendsto minimize the image size by excluding unnecessary packages.- 15-18: Creating the virtual environment directly in the root directory (
/venv) could lead to potential conflicts. Consider using a less generic directory..github/workflows/build.yml (18)
- 5-9: Adding
pull_request_reviewevents with specific types is a good practice to ensure that builds are triggered by meaningful actions in the workflow.- 13-13: The conditional logic for the build job ensures that builds are only triggered for approved pull requests targeting the master branch or manual dispatches, which is a good practice for maintaining a clean master branch.
- 16-20: Setting up a local registry service as part of the CI pipeline is a good practice for testing image builds and pushes within the workflow.
- 28-42: Implementing caching for GraphBLAS based on the platform and file hashes is an efficient way to speed up builds by reusing previously compiled binaries.
- 43-48: Caching the parser binaries is another example of optimizing the build process by avoiding unnecessary recompilation.
- 50-55: Caching search-static binaries follows the same principle of build optimization and is correctly implemented.
- 59-62: Relocating the source to a consistent directory is a good practice for ensuring that the build environment is predictable and stable across runs.
- 64-71: Setting up QEMU and Docker Buildx with network host driver options is correctly done to support cross-platform builds and local registry pushes.
- 73-84: Building the compiler image with the correct context, Dockerfile, and platform tags, and pushing it to the local registry is well-implemented for a multi-stage build process.
- 86-93: Copying binaries from the Docker container for caching when cache hits are missed is a smart way to ensure that subsequent builds can leverage these binaries, reducing build times.
- 95-104: Building the tests image with the correct base image and platform-specific arguments is crucial for ensuring that the testing environment is consistent with the build environment.
- 106-111: Uploading the tests image as an artifact for subsequent jobs to use is a good practice for ensuring that all tests run against the same image version.
- 113-124: Building the base image with platform-specific arguments and pushing it to the local registry is correctly implemented, ensuring that the final image is ready for deployment.
- 126-131: Uploading the final base image as an artifact ensures that it can be easily accessed for further actions, such as deployment or additional testing.
- 134-156: The setup for downloading and loading the test image before running unit tests ensures that tests are executed in a consistent and controlled environment.
- 159-181: Similarly, the setup for flow tests, including downloading and loading the test image, is correctly implemented to ensure test consistency.
- 184-206: The setup for TCK tests follows the same best practices for ensuring that tests are run against the correct image version, ensuring consistency across test runs.
- 209-231: The setup for fuzz tests, including the specific timeout setting, is well-implemented to ensure that fuzzing is performed in a controlled and consistent manner.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/build.yml (1 hunks)
Additional comments: 10
.github/workflows/build.yml (10)
- 13-13: The conditional check for pull request reviews only considers
approvedstate. Ensure that this aligns with the workflow requirements, as it might not trigger on other review states.- 16-20: Using a local registry service is a good practice for testing image builds. Ensure the registry is secured and not exposed unintentionally.
- 36-42: Caching strategy for GraphBLAS is well-implemented. Verify the hashFiles path to ensure it correctly identifies changes in GraphBLAS dependencies.
- 43-48: Similar to GraphBLAS, ensure the
hashFilespath for the parser cache key accurately reflects changes in parser dependencies.- 50-55: For the search cache, confirm the
hashFilespath is correctly set to detect changes in search dependencies.- 61-62: Ensure the directory move does not affect other jobs or steps that might rely on the original workspace location.
- 73-85: When building the compiler image, verify that the
TARGETPLATFORMbuild argument is correctly utilized in the Dockerfile to ensure multi-platform support.- 95-106: For the tests image build step, ensure the
BASE_IMAGEbuild argument is correctly used in the Dockerfile to build on top of the compiler image.- 114-125: Similar to the tests image, verify the
BASE_IMAGEbuild argument for the base image build step to ensure it correctly builds on top of the compiler image.- 135-157: For each test job (
unit-tests,flow-tests,tck-tests,fuzz-tests), ensure the downloaded and loaded test image is correctly utilized in the test execution steps.
added 2 commits
February 7, 2024 12:43
- add concurrency block - remove unused dependencies - refactored
added 2 commits
February 7, 2024 14:26
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- .github/workflows/build.yml (1 hunks)
- tests/Dockerfile (1 hunks)
- tests/fuzz/process.py (1 hunks)
- tests/requirements.txt (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/fuzz/process.py
- tests/requirements.txt
Additional comments: 22
tests/Dockerfile (5)
- 3-5: Ensure that the
TARGETPLATFORMargument is used consistently throughout the Dockerfile to support multi-platform builds.- 13-13: Use
apt-get install -y --no-install-recommendsto minimize the image size by excluding unnecessary packages.- 15-18: Creating the virtual environment directly in the root directory (
/venv) could lead to potential conflicts. Consider using a less generic directory.- 21-22: Upgrading
pipandsetuptoolsis a good practice to ensure the latest features and security fixes are used.- 25-25: Ensure the
requirements.txtfile exists at the specified path and contains all necessary packages for testing.Verification successful
The verification process has successfully identified the existence of a
requirements.txtfile within thetestsdirectory at./tests/requirements.txt. This confirms that therequirements.txtfile exists at the specified path and is likely to contain all necessary packages for testing, as indicated by the initial review comment.Given this confirmation, the review comment regarding the need to ensure the
requirements.txtfile exists at the specified path and contains all necessary packages for testing is verified as accurate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and content of requirements.txt in the tests directory. fd requirements.txt --exec cat src/FalkorDB/tests/requirements.txtLength of output: 525
Script:
#!/bin/bash # Correctly verify the existence and content of requirements.txt in the FalkorDB/tests directory. fd requirements.txt tests/FalkorDB --exec cat {}Length of output: 154
Script:
#!/bin/bash # General search for requirements.txt to find its actual location within the repository. fd requirements.txt --exec echo Found at: {}Length of output: 163
.github/workflows/build.yml (17)
- 5-10: Adding triggers for
pull_requestandpushevents on themasterbranch is a good practice to ensure the workflow runs on relevant changes.- 11-13: Implementing concurrency with cancellation of in-progress jobs helps to manage resources efficiently and avoid redundant builds.
- 19-23: Configuring a local registry service is a good approach for testing image builds and pushes within the CI environment.
- 30-31: Using an environment variable to handle architecture-specific suffixes enhances the workflow's flexibility for multi-platform builds.
- 33-40: Ensuring the working directory is safe for Git operations is important for security, especially when working with submodules.
- 41-46: Caching the compiler image based on the Dockerfile and dependencies hash is a good practice to speed up builds.
- 48-54: Relocating the source to a static directory is a good strategy for consistency, especially for caching and build tools.
- 55-62: Setting up QEMU and Docker Buildx supports cross-platform builds, which is essential for a project targeting multiple architectures.
- 78-82: Loading and pushing the compiler image to the local registry is necessary for subsequent steps. Ensure the registry service is correctly set up and accessible.
- 83-93: Building the tests image with the correct base image and target platform supports consistent testing environments across architectures.
- 95-101: Uploading the tests image as an artifact ensures it can be used in subsequent jobs, facilitating a modular and efficient CI/CD pipeline.
- 102-113: Building the base image with arguments for the base compiler image and target platform is consistent with best practices for Docker multi-stage builds.
- 115-120: Uploading the base image as an artifact is a good practice, ensuring it's available for deployment or further testing.
- 123-151: Downloading and loading the test image before running unit tests ensures that the correct version of the image is used for testing.
- 154-182: The setup for flow tests mirrors the unit tests setup, ensuring consistency across test types. Good practice.
- 185-213: TCK tests setup follows the established pattern for test execution, maintaining consistency and ensuring the correct test environment.
- 216-244: Fuzz tests are correctly set up with a timeout, ensuring that tests complete within a reasonable timeframe and resources are managed efficiently.
| @@ -1,23 +1,25 @@ | |||
| FROM redis:latest as builder | |||
| ARG BASE_IMAGE=falkordb/falkordb-compiler | |||
There was a problem hiding this comment.
Specify a fixed version for BASE_IMAGE instead of using the default tag to ensure reproducible builds.
|
|
||
| FROM --platform=$TARGETPLATFORM $BASE_IMAGE as compiler | ||
|
|
||
| ENV DEPS "automake peg libtool autoconf python-is-python3 2to3 python3.10-venv python3-pip wget build-essential cmake m4 git valgrind gcc python3-dev" |
There was a problem hiding this comment.
Consider grouping common dependencies to reduce the number of layers and optimize the Docker image size.
.github/workflows/build.yml
Outdated
Comment on lines
64
to
77
| - name: Build compiler image | ||
| if: steps.cache_compiler_image.outputs.cache-hit == false | ||
| id: build_compiler | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| context: /FalkorDB | ||
| file: /FalkorDB/build/docker/Dockerfile.compiler | ||
| platforms: ${{ matrix.platform }} | ||
| tags: localhost:5000/falkordb/falkordb-compiler | ||
| outputs: type=docker,dest=/tmp/falkordb-compiler-${{ env.ARCH_SUFFIX }}.tar | ||
| # push: true | ||
| build-args: | | ||
| TARGETPLATFORM=${{ matrix.platform }} | ||
|
|
There was a problem hiding this comment.
Building the compiler image conditionally based on cache hit status is efficient. However, ensure that the push option is correctly configured based on the CI/CD pipeline requirements.
- # push: true
+ push: ${{ github.event_name == 'push' && github.ref == 'refs/heads/master' }}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| - name: Build compiler image | |
| if: steps.cache_compiler_image.outputs.cache-hit == false | |
| id: build_compiler | |
| uses: docker/build-push-action@v5 | |
| with: | |
| context: /FalkorDB | |
| file: /FalkorDB/build/docker/Dockerfile.compiler | |
| platforms: ${{ matrix.platform }} | |
| tags: localhost:5000/falkordb/falkordb-compiler | |
| outputs: type=docker,dest=/tmp/falkordb-compiler-${{ env.ARCH_SUFFIX }}.tar | |
| # push: true | |
| build-args: | | |
| TARGETPLATFORM=${{ matrix.platform }} | |
| - name: Build compiler image | |
| if: steps.cache_compiler_image.outputs.cache-hit == false | |
| id: build_compiler | |
| uses: docker/build-push-action@v5 | |
| with: | |
| context: /FalkorDB | |
| file: /FalkorDB/build/docker/Dockerfile.compiler | |
| platforms: ${{ matrix.platform }} | |
| tags: localhost:5000/falkordb/falkordb-compiler | |
| outputs: type=docker,dest=/tmp/falkordb-compiler-${{ env.ARCH_SUFFIX }}.tar | |
| push: ${{ github.event_name == 'push' && github.ref == 'refs/heads/master' }} | |
| build-args: | | |
| TARGETPLATFORM=${{ matrix.platform }} |
AviAvni
approved these changes
Feb 7, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- .github/workflows/build.yml (1 hunks)
Additional comments: 14
.github/workflows/build.yml (14)
- 11-13: The concurrency group expression is well-formed, ensuring that runs are queued per PR or branch, effectively preventing race conditions.
- 19-23: Using a local registry service is a good practice for testing image builds and pushes within the CI environment.
- 30-31: The use of
ARCH_SUFFIXenvironment variable to handle architecture-specific builds is a clean approach for cross-platform compatibility.- 41-61: Caching dependencies based on hash files is a best practice for speeding up builds. Ensure that the paths and keys are correctly specified for effective caching.
- 66-68: Moving the workspace to a static directory is a good strategy for consistency, especially when dealing with caching and Docker builds.
- 91-99: Copying binaries from Docker for caching is a smart approach, but ensure that the conditional check for cache hits is correctly implemented to avoid unnecessary steps.
- 100-111: Building the tests image with the correct base image and platform is crucial for consistent test environments. Verify that the
BASE_IMAGEandTARGETPLATFORMare correctly utilized.- 112-117: Uploading the tests image as an artifact is a good practice for sharing between jobs. Ensure that the artifact name and path are correctly configured.
- 119-131: The base image build step is correctly set up. Pay attention to the use of
BASE_IMAGEandTARGETPLATFORMfor consistency across platforms.- 132-137: Uploading the base image as an artifact follows best practices for artifact management. Confirm that the artifact configuration is correct.
- 140-168: The setup for unit tests, including downloading the image artifact and running tests in a Docker container, is correctly implemented. Ensure that the platform-specific configurations are correctly applied.
- 171-199: The flow tests setup mirrors the unit tests setup, with appropriate platform-specific configurations. This consistency across test types is good practice.
- 202-230: The TCK tests setup is consistent with other test setups, ensuring platform-specific configurations are applied. This consistency is beneficial for maintainability.
- 233-261: The fuzz tests setup, including downloading the image artifact and running tests in a Docker container, is correctly implemented. Ensure that the
TIMEOUTparameter is appropriately used.
Comment on lines
+5
to
+13
| pull_request: | ||
| branches: [master] | ||
| push: | ||
| branches: | ||
| - master | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
Consider adding a filter for types under pull_request to limit the workflow trigger to specific activities, enhancing control and efficiency.
Comment on lines
+78
to
+89
| - name: Build compiler image | ||
| id: build_compiler | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| context: /FalkorDB | ||
| file: /FalkorDB/build/docker/Dockerfile.compiler | ||
| platforms: ${{ matrix.platform }} | ||
| tags: localhost:5000/falkordb/falkordb-compiler | ||
| # outputs: type=docker,dest=/tmp/falkordb-compiler.tar | ||
| push: true | ||
| build-args: | | ||
| TARGETPLATFORM=${{ matrix.platform }} |
There was a problem hiding this comment.
The build process for the compiler image is correctly configured. However, consider the previous comment about caching build files instead of the build itself for potentially better efficiency.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits the build pipeline into 2, one for building and testing, and another for releasing the image to Docker hub when a Release is created.
Summary by CodeRabbit