-
Notifications
You must be signed in to change notification settings - Fork 174
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: Run unit test + downstream on main ubuntu build (gitlab) #4020
ci: Run unit test + downstream on main ubuntu build (gitlab) #4020
Conversation
Caution Review failedThe pull request is closed. WalkthroughChanges to the GitLab CI configuration, there are. Enhance the build process for Linux Ubuntu, these modifications shall. New commands added to run integration tests, install the project, and configure a downstream project, they have. Comprehensive testing and build workflow, this represents - expanding the continuous integration capabilities, yes. Changes
Suggested labels
Suggested reviewers
Possibly related PRs
No Sequence Diagram NeededComplexity of sequence not high enough, a diagram would be, hmm. Simple CI configuration changes, these are. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitlab-ci.yml (1)
244-248
: Handle failures gracefully, a Jedi must.Missing error handling for the version check, I sense. Add it, we should.
Apply this change to handle errors:
- # Downstream run - - ./build-downstream/bin/ShowActsVersion + # Downstream run with error handling + - | + if ! ./build-downstream/bin/ShowActsVersion; then + echo "Failed to run ShowActsVersion. Investigate the installation we must." + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.gitlab-ci.yml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: linux_ubuntu
- GitHub Check: docs
🔇 Additional comments (3)
.gitlab-ci.yml (3)
229-230
: Wise choice, running tests in parallel is! Hmmmm.Efficient use of resources, I sense. Integration tests separately built, they are. Good practices, these changes follow.
232-233
: Install the project properly, we must.Clear and necessary, this installation step is. For downstream project, essential it becomes.
235-242
: Verify the installation path, we should.Comprehensive, the downstream configuration is. But verify the
INSTALL_DIR
variable's presence and value, we must.Run this command to check the variable's definition:
✅ Verification successful
Properly defined, the installation path is.
In both GitLab and GitHub realms, correctly set the
INSTALL_DIR
variable is. Peace of mind, this brings to our downstream configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if INSTALL_DIR is properly defined rg -A 1 "INSTALL_DIR:" .gitlab-ci.ymlLength of output: 134
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.gitlab-ci.yml (2)
229-230
: Preserve the test results, you must!Wise to run tests in parallel, it is. But artifacts for test results, we should keep, hmm.
Add this configuration, you shall:
- ctest --test-dir build -j$(nproc) + - ctest --test-dir build -j$(nproc) --output-junit test_results.xml - cmake --build build --target integrationtests
232-233
: Clear path to installation, define we should.Correct the installation step is, but explicit installation path, specify we must.
Modify thus, you shall:
# Install main project - - cmake --install build + - cmake --install build --prefix "${INSTALL_DIR}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.gitlab-ci.yml
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204_clang, 20)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20)
- GitHub Check: docs
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
🔇 Additional comments (1)
.gitlab-ci.yml (1)
235-241
: Strong with the Force, this configuration is.Wise choices made, I see. Ninja generator for speed, warnings as errors for quality, and correct path for dependencies, you have chosen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't have a huge impact on the performance. Looks good!
Quality Gate passedIssues Measures |
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat
,fix
,refactor
,docs
,chore
andbuild
types.Summary by CodeRabbit