ci: remove nix from gh workflows #21572
Conversation
WalkthroughWalkthroughThis pull request introduces a shell script for automating the installation of the RocksDB database library on Unix-like systems, along with modifications to GitHub Actions workflows for build, linting, and testing processes. It includes version control for RocksDB, enhances caching strategies, and simplifies the setup and execution of tests. Additionally, it updates the Go version used in the workflows and modifies linting scripts to accommodate the new RocksDB integration. Changes
Possibly related PRs
Suggested labels
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
| @@ -14,8 +15,6 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| steps: | |||
| - uses: actions/checkout@v4 | |||
| - uses: DeterminateSystems/nix-installer-action@main | |||
There was a problem hiding this comment.
YES, thank you!!
We would be able to remove so much boilerplate from scripts, ci etc..
julienrbrt
added
the
backport/v0.52.x
auricom
force-pushed
the
claude/gomod2nix
branch
2 times, most recently
from
8b90c9d
to
8b071f1
Compare
.github/workflows/build.yml
Outdated
| path: /usr/local/lib | ||
| key: ${{ runner.os }}-rocksdb-${{ matrix.go-arch }} | ||
| - name: install rocksdb | ||
| if: env.GIT_DIFF && steps.cache-rocksdb.outputs.cache-hit != 'true' |
There was a problem hiding this comment.
As rocksdb compilation lasts ~8mn on public runners, it will be executed only when cache is invalidated by github (should be once every 7 days OR when changing rocksdb version)
.github/workflows/build.yml
Outdated
| sudo apt update && sudo apt-get install -y zlib1g-dev libbz2-dev libzstd-dev build-essential | ||
| git clone https://github.com/facebook/rocksdb.git /home/runner/rocksdb | ||
| cd /home/runner/rocksdb || exit 1 | ||
| git checkout "v$ROCKSDB_VERSION" | ||
| sudo make -j "$(nproc --all)" shared_lib && sudo ldconfig |
There was a problem hiding this comment.
rocksdb is now installed directly on public runners. it is made available on /usr/local/lib
auricom
force-pushed
the
claude/gomod2nix
branch
2 times, most recently
from
6dd64be
to
54cfe68
Compare
auricom
force-pushed
the
claude/gomod2nix
branch
2 times, most recently
from
7e53f8a
to
7a93d73
Compare
auricom
force-pushed
the
claude/gomod2nix
branch
from
39f32b1
to
7f81e30
Compare
auricom
force-pushed
the
claude/gomod2nix
branch
from
7f81e30
to
02efb47
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (3)
.github/workflows/lint.yml (1)
33-34: Fix Permissions for Cache: Potential Security ImplicationsSetting permissions with
sudo chownto allow the current user to access system directories can have security implications. Consider using a safer approach or ensuring that this change is well-documented and understood by all stakeholders.Tools
actionlint
34-34: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
.github/workflows/test.yml (2)
793-794: Fix Permissions for Cache: Potential Security ImplicationsAs noted in the lint workflow, setting permissions with
sudo chownto allow the current user to access system directories can have security implications. Consider using a safer approach or ensuring that this change is well-documented and understood by all stakeholders.Tools
actionlint
794-794: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
849-849: Fix Permissions for Cache: Potential Security ImplicationsAs noted in previous workflows, setting permissions with
sudo chownto allow the current user to access system directories can have security implications. Consider using a safer approach or ensuring that this change is well-documented and understood by all stakeholders.Tools
actionlint
849-849: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
Files selected for processing (6)
- .github/scripts/install-rocksdb.sh (1 hunks)
- .github/workflows/build.yml (3 hunks)
- .github/workflows/lint.yml (3 hunks)
- .github/workflows/test.yml (5 hunks)
- scripts/go-lint-all.bash (1 hunks)
- scripts/go-mod-tidy-all.sh (1 hunks)
Files skipped from review due to trivial changes (1)
- scripts/go-mod-tidy-all.sh
Additional context used
actionlint
.github/workflows/build.yml
32-32: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
.github/workflows/lint.yml
34-34: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
.github/workflows/test.yml
794-794: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
849-849: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
Additional comments not posted (22)
.github/scripts/install-rocksdb.sh (1)
2-2: Good use ofset -efor error handling.This command ensures that the script will exit immediately if a command exits with a non-zero status, which is a good practice in shell scripts to prevent cascading failures.
scripts/go-lint-all.bash (1)
9-9: Conditional logic forROCKSDBenvironment variable.The script now checks if the
ROCKSDBenvironment variable is set and appends,rocksdbtoLINT_TAGSif true. This is a sensible change that allows conditional inclusion of RocksDB-related linting tags based on the environment configuration. Ensure that theROCKSDBvariable is properly set in environments where RocksDB-specific linting is required..github/workflows/build.yml (3)
16-18: Addition ofROCKSDB_VERSIONenvironment variable.The introduction of the
ROCKSDB_VERSIONenvironment variable is a good practice for managing the version of RocksDB used across different builds. This ensures consistency and traceability of the build environment.
31-53: Enhanced caching and installation steps for RocksDB.The workflow now includes steps to fix permissions, restore RocksDB libraries from cache, and install RocksDB if not cached. These steps are crucial for optimizing build times and ensuring that the RocksDB installation is handled correctly across builds. However, ensure that the cache keys are correctly configured to avoid potential issues with cache invalidation.
key: ${{ runner.os }}-rocksdb-${{ env.ROCKSDB_VERSION }}-${{ matrix.go-arch }}Tools
actionlint
32-32: shellcheck reported issue in this script: SC2046:warning:1:12: Quote this to prevent word splitting
(shellcheck)
62-63: Simplified conditional logic for building with RocksDB backend.The conditional logic to build with the RocksDB backend has been simplified to only check if the architecture is
amd64. This makes the workflow easier to understand and maintain. Ensure that this condition correctly reflects the project's requirements for building with RocksDB..github/workflows/lint.yml (8)
13-13: Environment Variable Added: ROCKSDB_VERSIONThe addition of the
ROCKSDB_VERSIONenvironment variable is a good practice for managing library versions centrally, which simplifies updates and ensures consistency across CI jobs.
36-42: Restore RocksDB Libraries Cache: Well-ImplementedThis step efficiently handles the caching of RocksDB libraries, which should speed up the CI process by avoiding unnecessary reinstalls. The use of specific cache keys based on the OS and RocksDB version is a good practice.
44-46: Conditional Installation of RocksDB: Good Use of Conditional StepsThe conditional logic to check the cache hit status before proceeding with the installation is a smart way to optimize CI runtime. This ensures that RocksDB is only installed when necessary.
48-54: Saves RocksDB Libraries Cache: Effective Caching StrategyThis step effectively saves the RocksDB libraries to the cache after a successful installation, which is crucial for optimizing future runs. The configuration is consistent and uses the same key structure as the restore step.
59-59: Simplified Lint Command: Direct Call to MakeReplacing
nix develop -c make lintwithmake lintsimplifies the workflow and reduces the complexity previously introduced by Nix. This change aligns with the PR's goal of simplifying the CI process.
61-61: Environment Variable Modification: NIX to ROCKSDBChanging the environment variable from
NIXtoROCKSDBreflects the shift from Nix-based setups to direct management of RocksDB, supporting the PR's objectives.
88-88: Environment Variable Consistency: ROCKSDBThe consistent use of the
ROCKSDBenvironment variable across different steps helps maintain clarity and ensures that all parts of the workflow are aligned with the new setup.
84-84: Repeated Lint Command: Ensure NecessityThe lint command is repeated in multiple steps. Ensure that this repetition is necessary and that each instance serves a specific purpose, possibly related to different conditions or setups.
Verification successful
Repeated Lint Command: Justified by Different Conditions
The repetition of the
make lintcommand in the workflow file is justified by the different conditions and environment setups associated with each instance. Each command serves a specific purpose, tailored to different scenarios in the CI process.
- The first instance is configured with
ROCKSDB: 1.- The second instance is conditional on
steps.lint_long.outcome == 'skipped'and usesGIT_DIFFandLINT_DIFF: 1.- The third instance combines
GIT_DIFF,LINT_DIFF: 1, andROCKSDB: 1.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the necessity of repeated lint commands. # Test: Search for repeated lint commands. Expect: Justification for each repetition. rg --type yaml -A 5 $'make lint' .github/workflows/lint.ymlLength of output: 549
.github/workflows/test.yml (9)
17-17: Environment Variable Added: ROCKSDB_VERSIONThe addition of the
ROCKSDB_VERSIONenvironment variable is consistent with the changes in the lint workflow and is a good practice for managing library versions centrally.
795-802: Restore RocksDB Libraries Cache: Well-ImplementedThis step efficiently handles the caching of RocksDB libraries, which should speed up the CI process by avoiding unnecessary reinstalls. The use of specific cache keys based on the OS and RocksDB version is a good practice.
804-806: Conditional Installation of RocksDB: Good Use of Conditional StepsThe conditional logic to check the cache hit status before proceeding with the installation is a smart way to optimize CI runtime. This ensures that RocksDB is only installed when necessary.
808-814: Saves RocksDB Libraries Cache: Effective Caching StrategyThis step effectively saves the RocksDB libraries to the cache after a successful installation, which is crucial for optimizing future runs. The configuration is consistent and uses the same key structure as the restore step.
819-819: Test Command: Direct Call to Go TestThe direct invocation of
go testwith specific tags and coverage options simplifies the testing process and aligns with the PR's goal of simplifying the CI setup by removing Nix.
850-857: Restore RocksDB Libraries Cache: Well-ImplementedThis step efficiently handles the caching of RocksDB libraries, which should speed up the CI process by avoiding unnecessary reinstalls. The use of specific cache keys based on the OS and RocksDB version is a good practice.
859-861: Conditional Installation of RocksDB: Good Use of Conditional StepsThe conditional logic to check the cache hit status before proceeding with the installation is a smart way to optimize CI runtime. This ensures that RocksDB is only installed when necessary.
863-869: Saves RocksDB Libraries Cache: Effective Caching StrategyThis step effectively saves the RocksDB libraries to the cache after a successful installation, which is crucial for optimizing future runs. The configuration is consistent and uses the same key structure as the restore step.
874-874: Test Command: Direct Call to Go TestThe direct invocation of
go testwith specific tags and coverage options simplifies the testing process and aligns with the PR's goal of simplifying the CI setup by removing Nix.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- .github/scripts/install-rocksdb.sh (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/scripts/install-rocksdb.sh
If we hard-code the rocksdb version in the script, that is essentially the same thing. |
(cherry picked from commit c536c0f) # Conflicts: # .github/workflows/lint.yml
Description
Objective is to remove nix, that was required for rocksdb libraries.
This will help reducing complexity and streamline the ci experience.
rocksdb libraries are now installed directly on public ubuntu runners, then cached to improve execution time.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor