fix: ctest execute fail，remove duplicate ci #57
Conversation
WalkthroughThe pull request introduces modifications to two GitHub Actions workflow configuration files. In Changes
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
.github/workflows/import_braft.yml (1)
Line range hint
10-67: Consider optimizing the workflow structure.While the jobs themselves haven't changed, there are a few suggestions for potential improvements:
The
build_on_macosandbuild_on_ubuntujobs have similar structures. Consider using a matrix strategy to reduce duplication and make it easier to add more platforms in the future.The Go E2E tests are run separately for macOS and Ubuntu. If these tests are platform-independent, consider running them only once to save time and resources.
The build scripts (
./etc/script/ci/build.shand./etc/script/build.sh) are different for the format check and the actual builds. It might be beneficial to standardize these scripts if possible.Here's a sample of how you could use a matrix strategy to simplify the workflow:
jobs: check_format: # ... (unchanged) build_and_test: needs: check_format strategy: matrix: os: [ubuntu-latest, macos-latest] runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 - name: Set up environment (macOS) if: matrix.os == 'macos-latest' run: | brew install autoconf brew install go - name: Build env: CPLUS_INCLUDE_PATH: ${{ matrix.os == 'macos-latest' && '/opt/homebrew/include' || '' }} run: bash ./etc/script/build.sh --verbose - name: Run Go E2E Tests working-directory: ${{ github.workspace }}/build-release run: | cd ../tests go mod tidy go test ./kiwi_suite_test.go ./consistency_test.go -vThis suggestion consolidates the build and test steps into a single job with a matrix strategy, which can make the workflow more maintainable and easier to extend in the future.
.github/workflows/pikiwidb.yml (2)
9-11: LGTM: Introduction of BUILD_DIR environment variableThe addition of the
BUILD_DIRenvironment variable is a good practice. It centralizes the build directory configuration, enhancing maintainability and reducing the risk of inconsistencies across the workflow.Consider adding a comment explaining the purpose of this variable, e.g.:
env: # Directory for CMake release builds BUILD_DIR: cmake-build-release
Line range hint
1-88: Overall assessment and recommendationsThe changes to this workflow file generally improve the CI configuration by centralizing the build directory setting and making the workflow more maintainable. These modifications align well with the PR objectives of fixing CI issues.
However, there are consistent issues with the naming of environment variables throughout the file. The variable is defined as
BUILD_DIR, but often referenced asBUILD_TYPE. This inconsistency needs to be addressed to prevent potential CI failures.Recommendations:
- Conduct a global search and replace operation to ensure all instances of
BUILD_TYPEare changed toBUILD_DIR.- After making these changes, thoroughly test the workflow to ensure it runs correctly with the updated variable names.
- Consider adding comments to explain the purpose of key environment variables and job steps to improve maintainability.
These changes will significantly improve the reliability and maintainability of your CI workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/import_braft.yml (1 hunks)
- .github/workflows/pikiwidb.yml (6 hunks)
🧰 Additional context used
🔇 Additional comments (2)
.github/workflows/import_braft.yml (1)
Line range hint
1-8: LGTM: Workflow trigger updated correctly.The change to include the
pushevent for the "import-braft" branch is consistent with the existingpull_requestconfiguration. This update ensures that the workflow runs on both push events to the specified branch and pull requests targeting it, which aligns with common CI practices.To ensure this change doesn't conflict with other workflow files or branch protection rules, let's run the following check:
.github/workflows/pikiwidb.yml (1)
5-5: LGTM: Addition of "unstable" branch triggerThe addition of the "unstable" branch to the push trigger is appropriate. This ensures that the workflow runs when changes are pushed to the development branch, aligning with common CI practices.
.github/workflows/pikiwidb.yml
Outdated
| @@ -33,7 +37,7 @@ jobs: | |||
| bash ./etc/script/build.sh --verbose | |||
|
|
|||
| - name: GTest | |||
| working-directory: ${{ github.workspace }}/build-release | |||
| working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} | |||
There was a problem hiding this comment.
Fix inconsistent environment variable usage in build_on_macos job
The working directory paths have been updated to use environment variables, which is good for maintainability. However, there's an inconsistency in the variable names used here compared to the earlier definition.
Please update these lines to use the correct environment variable name:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }}
+ working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}This change should be applied to both the "GTest" step (line 40) and the "Run Go E2E Tests" step (line 51).
Also applies to: 51-51
.github/workflows/pikiwidb.yml
Outdated
| @@ -16,7 +20,7 @@ | |||
| run: bash ./etc/script/ci/build.sh | |||
|
|
|||
| - name: Check Format | |||
| working-directory: ${{ github.workspace }}/build | |||
| working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR}} | |||
There was a problem hiding this comment.
Fix inconsistent environment variable usage
The working directory path has been updated to use the new environment variable, which is good for consistency. However, there's an inconsistency in the variable name used here compared to other parts of the file.
Please update the line to use the correct environment variable name:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR}}
+ working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}Also, note that in other parts of the file, BUILD_TYPE is used instead of BUILD_DIR. Ensure that the correct variable name is used consistently throughout the file.
Committable suggestion was skipped due to low confidence.
.github/workflows/pikiwidb.yml
Outdated
| @@ -64,7 +68,7 @@ | |||
| bash ./etc/script/build.sh --verbose | |||
|
|
|||
| - name: GTest | |||
| working-directory: ${{ github.workspace }}/build-release | |||
| working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} | |||
There was a problem hiding this comment.
💡 Codebase verification
Update all instances of BUILD_TYPE to BUILD_DIR in .github/workflows/pikiwidb.yml
Multiple instances of BUILD_TYPE were found in .github/workflows/pikiwidb.yml. Please update all occurrences to use BUILD_DIR to maintain consistency.
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }}
+ working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}This change should be applied to all affected lines.
🔗 Analysis chain
Fix inconsistent environment variable usage in build_on_ubuntu job
The working directory paths have been updated to use environment variables, which is consistent with the changes in the macos job. However, the same inconsistency in variable names is present here.
Please update these lines to use the correct environment variable name:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }}
+ working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}This change should be applied to both the "GTest" step (line 71) and the "Run Go E2E Tests" step (line 82).
To ensure all instances of this issue are addressed, run the following command:
If the command returns any results, those lines should also be updated to use BUILD_DIR.
Also applies to: 82-82
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of BUILD_TYPE in the workflow file
rg --type yaml 'BUILD_TYPE' .github/workflows/pikiwidb.yml
Length of output: 350
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pikiwidb.yml (1)
9-11: LGTM: Introduced BUILD_DIR environment variableAdding the
BUILD_DIRenvironment variable is a good practice for centralizing the build directory configuration. This improves maintainability and consistency across the workflow.However, consider the following suggestion:
To make the configuration more flexible, you might want to consider using a GitHub Actions input parameter instead of a hardcoded value. This would allow you to easily change the build directory for different workflow runs. For example:
env: BUILD_DIR: ${{ inputs.build_dir || 'cmake-build-release' }}This way, you can provide a default value while still allowing for customization when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/pikiwidb.yml (5 hunks)
🧰 Additional context used
🔇 Additional comments (4)
.github/workflows/pikiwidb.yml (4)
5-5: LGTM: Added "unstable" branch to workflow triggersAdding the "unstable" branch to the push triggers is a good practice. This ensures that the workflow runs on pushes to the development branch, allowing for continuous integration and early detection of issues.
Line range hint
1-87: Summary of review for .github/workflows/pikiwidb.ymlThe changes made to this workflow file generally improve its flexibility and align with the PR objectives. Here's a summary of the main points:
- The addition of the "unstable" branch to the workflow triggers is approved.
- The introduction of the
BUILD_DIRenvironment variable is a good practice for centralized configuration.- There are inconsistencies in the usage of environment variables (
BUILD_TYPEvsBUILD_DIR) that need to be addressed in both thebuild_on_macosandbuild_on_ubuntujobs.- A verification step is recommended to ensure all instances of
BUILD_TYPEare replaced withBUILD_DIRthroughout the file.Once these issues are addressed, the workflow will be more consistent and maintainable. Good job on improving the CI configuration!
40-40:⚠️ Potential issueFix inconsistent environment variable usage in build_on_macos job
The working directory paths have been updated to use environment variables, which is good for maintainability. However, there's an inconsistency in the variable names used here compared to the earlier definition.
As mentioned in a previous review, please update these lines to use the correct environment variable name:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} + working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}This change should be applied to both the "GTest" step (line 40) and the "Run Go E2E Tests" step (line 51).
Also applies to: 51-51
71-71:⚠️ Potential issueFix inconsistent environment variable usage in build_on_ubuntu job
The working directory paths have been updated to use environment variables, which is consistent with the changes in the macos job. However, the same inconsistency in variable names is present here.
As mentioned in a previous review, please update these lines to use the correct environment variable name:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} + working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}This change should be applied to both the "GTest" step (line 71) and the "Run Go E2E Tests" step (line 82).
Also applies to: 82-82
| pull_request: | ||
| branches: [ "unstable" ] | ||
|
|
||
| env: | ||
| BUILD_DIR: cmake-build-release |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of BUILD_DIR identified
The environment variable BUILD_TYPE is still used in the following lines and should be updated to BUILD_DIR for consistency:
- Line 40
- Line 51
- Line 71
- Line 82
🔗 Analysis chain
Verify consistent usage of BUILD_DIR throughout the file
To ensure all instances of the environment variable are correctly updated, please run the following verification:
If the command returns any results, those lines should be updated to use BUILD_DIR instead of BUILD_TYPE. This will maintain consistency with the environment variable definition at the top of the file.
Also applies to: 40-40, 51-51, 71-71, 82-82
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of BUILD_TYPE in the workflow file
rg --type yaml 'BUILD_TYPE' .github/workflows/pikiwidb.yml
Length of output: 350
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pikiwidb.yml (1)
Line range hint
1-92: Summary of changes and suggestions for improvementThe changes in this PR improve the workflow by:
- Adding the "unstable" branch to workflow triggers.
- Introducing an environment variable for build directory management.
- Updating working directories and test execution commands for better consistency.
However, there are some issues that need to be addressed:
- Inconsistent environment variable naming:
BUILD_DIRis defined butBUILD_TYPEis used throughout the file. This needs to be fixed for all occurrences.- The PR objective of removing duplicate CI configurations is not clearly addressed in these changes.
To fully meet the PR objectives and improve the workflow:
- Fix all instances of
BUILD_TYPEto useBUILD_DIRas suggested in the previous comments.- Review the entire CI/CD pipeline to identify and remove any duplicate configurations that may exist in other workflow files or within this file.
- Consider adding comments to explain the purpose of the
BUILD_DIRvariable and any other non-obvious configurations to improve maintainability.- Verify that these changes resolve the ctest execution failure mentioned in the PR title.
Once these issues are addressed, the PR will be in a good state for approval.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/pikiwidb.yml (3 hunks)
🧰 Additional context used
🔇 Additional comments (6)
.github/workflows/pikiwidb.yml (6)
5-7: LGTM: Added "unstable" branch to workflow triggersThe addition of the "unstable" branch to both push and pull_request triggers is appropriate and aligns with the PR objectives. This change ensures that the workflow runs on the desired branches.
9-11:⚠️ Potential issueFix inconsistent environment variable usage
The introduction of the
BUILD_DIRenvironment variable is a good practice for maintainability. However, there's an inconsistency between this variable name and its usage in other parts of the file.Please update all occurrences of
BUILD_TYPEtoBUILD_DIRthroughout the file to maintain consistency. This includes lines 40, 51, 73, and 86.Example fix:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} + working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}
40-45:⚠️ Potential issueUpdate environment variable name and improve directory navigation
The use of an environment variable for the working directory and the addition of a
cdcommand beforemake testare good improvements. However, there's an inconsistency in the environment variable name.Please make the following changes:
- Update the environment variable name from
BUILD_TYPEtoBUILD_DIRto match the earlier definition.- Simplify the directory navigation by using the working-directory directly.
Apply this diff:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} + working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }} run: | - cd ${{ env.BUILD_TYPE }} make test
53-53:⚠️ Potential issueUpdate environment variable name for consistency
The use of an environment variable for the working directory is a good practice. However, there's an inconsistency in the environment variable name.
Please update the environment variable name from
BUILD_TYPEtoBUILD_DIRto match the earlier definition:- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} + working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}
73-78:⚠️ Potential issueUpdate environment variable name and improve directory navigation
The changes in this section mirror those in the build_on_macos job. While the improvements are good, there's still an inconsistency in the environment variable name.
Please make the following changes:
- Update the environment variable name from
BUILD_TYPEtoBUILD_DIRto match the earlier definition.- Simplify the directory navigation by using the working-directory directly.
Apply this diff:
- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} + working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }} run: | - cd ${{ env.BUILD_TYPE }} make test
86-86:⚠️ Potential issueUpdate environment variable name for consistency
The use of an environment variable for the working directory is consistent with the changes in the build_on_macos job. However, the inconsistency in the environment variable name persists.
Please update the environment variable name from
BUILD_TYPEtoBUILD_DIRto match the earlier definition:- working-directory: ${{ github.workspace }}/${{ env.BUILD_TYPE }} + working-directory: ${{ github.workspace }}/${{ env.BUILD_DIR }}
修复ctest没有执行, 优化ci流程,去掉重复的ci
fix:#33
Summary by CodeRabbit
New Features
BUILD_DIRfor improved configurability.Bug Fixes