Add PartCAD Dev Container #3
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive updates to the development container configurations for the PartCAD and Unobtainium projects. The changes span multiple files across different directories, focusing on enhancing development environment setup, tooling, and configuration. Key modifications include updating devcontainer configurations, adding new scripts for environment bootstrapping, introducing package management scripts, and refining GitHub Actions workflows. The changes aim to streamline the development process and provide a more robust, reproducible environment for both projects. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (14)
.github/workflows/test-pr.yaml (1)
7-7: Good practice using fixed Ubuntu version!Using
ubuntu-24.04instead ofubuntu-latestensures reproducible builds and prevents unexpected issues when Ubuntu releases new versions. This change aligns with infrastructure-as-code best practices.Consider documenting the Ubuntu version update process in the repository's maintenance guide to ensure timely updates when new LTS versions are released.
Also applies to: 20-20
src/partcad/.devcontainer/bootstrap.sh (3)
28-65: Add cleanup and handle GitHub API rate limits.The Allure installation function has two potential issues:
- The downloaded
.debfile in/tmp/allure.debis not cleaned up after installation- GitHub API requests are not authenticated, which could hit rate limits in high-usage scenarios
install_allure() { local allure_url + # Use GitHub token if available to avoid rate limits + local gh_header="" + if [[ -n "${GITHUB_TOKEN:-}" ]]; then + gh_header="Authorization: token ${GITHUB_TOKEN}" + fi + allure_url=$(curl -s https://api.github.com/repos/allure-framework/allure2/releases/latest | grep "browser_download_url.*allure_.*_all.deb" | cut -d '"' -f 4) if [[ -z "${allure_url}" ]]; then echo "Failed to fetch the latest Allure download URL" exit 1 fi local allure_deb="/tmp/allure.deb" echo "Downloading Allure..." if ! curl -JL -o "${allure_deb}" "${allure_url}"; then echo "Failed to download Allure" exit 1 fi # ... existing installation code ... echo "Allure installed successfully" + # Cleanup + rm -f "${allure_deb}" }
87-98: Reduce duplication in Poetry plugin installation verification.The plugin installation verification code is duplicated. Consider extracting it into a helper function.
+verify_poetry_plugin() { + local plugin="$1" + if ! poetry self show plugins | grep -q "${plugin}"; then + echo "Poetry plugin verification failed: ${plugin}" + return 1 + fi + return 0 +} + install_component "poetry export plugin" "poetry self add poetry-plugin-export" -# Verify poetry plugin installation -if ! poetry self show plugins | grep -q "poetry-plugin-export"; then - echo "Poetry plugin verification failed" - exit 1 -fi +verify_poetry_plugin "poetry-plugin-export" || exit 1 + install_component "poetry multiproject plugin" "poetry self add poetry-multiproject-plugin" -# Verify poetry plugin installation -if ! poetry self show plugins | grep -q "poetry-multiproject-plugin"; then - echo "Poetry plugin verification failed" - exit 1 -fi +verify_poetry_plugin "poetry-multiproject-plugin" || exit 1
1-125: Consider adding cleanup and version pinning.The script is well-structured but could benefit from:
- A cleanup function to handle interrupts and errors
- Version pinning for critical components to ensure reproducible environments
- A logging function for consistent message formatting
Example improvements:
# Add at the beginning of the script cleanup() { # Remove temporary files rm -f /tmp/allure.deb # Additional cleanup as needed } trap cleanup EXIT # Add version constants ALLURE_VERSION="2.24.1" # Example version PRE_COMMIT_VERSION="3.5.0" # Example version POETRY_EXPORT_PLUGIN_VERSION="^1.6.0" # Example version # Add logging function log() { local level="$1" shift echo "[${level}] $(date '+%Y-%m-%d %H:%M:%S') - $*" }test/partcad/test.sh (1)
11-28: Consider grouping tool checks by category.The tool verification section could be better organized by grouping related tools together (e.g., Python ecosystem tools, Git-related tools, etc.).
Here's a suggested reorganization:
# Python ecosystem check "python" [ "$(command -v python)" ] check "conda" [ "$(command -v conda)" ] check "poetry" [ "$(command -v poetry)" ] echo "poetry version: $(poetry --version)" check "pre-commit" [ "$(command -v pre-commit)" ] echo "pre-commit version: $(pre-commit --version)" # Git tools check "gh" [ "$(command -v gh)" ] check "git-lfs" [ "$(command -v git-lfs)" ] # Development tools check "hadolint" [ "$(command -v hadolint)" ] check "allure" [ "$(command -v allure)" ] echo "allure version: $(allure --version)" # Utilities check "jq" [ "$(command -v jq)" ] check "yq" [ "$(command -v yq)" ] check "starship" [ "$(command -v starship)" ]src/partcad/.devcontainer/apt-compile.sh (3)
1-13: LGTM! Consider enhancing error handling.The prerequisite checks for the apt.in file and root privileges are well implemented with appropriate exit codes.
Consider adding error output to stderr for better error handling:
- echo "Error: apt.in file not found." + echo "Error: apt.in file not found." >&2 - echo "Please run this script as root or using sudo." + echo "Please run this script as root or using sudo." >&2
15-19: Consider using a more explicit file clearing method.While
true >works, usingecho -n > "$OUTPUT_FILE"or: > "$OUTPUT_FILE"would be more explicit and commonly used.
48-51: Add output validation.Consider validating the output file to ensure it's not empty and contains valid package specifications.
sort -u -o "$OUTPUT_FILE" "$OUTPUT_FILE" +# Validate output file +if [[ ! -s "$OUTPUT_FILE" ]]; then + echo "Error: No packages were written to $OUTPUT_FILE" >&2 + exit 1 +fi + echo "Locked package versions have been written to $OUTPUT_FILE.".devcontainer/devcontainer.json (1)
54-72: Consider organizing tasks into categoriesThe pre-commit task is currently the only task defined. Consider adding more common development tasks and organizing them into categories.
"tasks": { "version": "2.0.0", "tasks": [ + { + "label": "Development", + "dependsOn": ["pre-commit: manual"] + }, { "label": "pre-commit: manual", "type": "shell",src/partcad/.devcontainer/ssh-sign.sh (1)
61-64: Consider making the signers file path configurableThe script hardcodes the signers file location. Consider making it configurable via an environment variable.
-SIGNERS_FILE="$GIT_DIR/.devcontainer/signers" +SIGNERS_FILE="${SIGNERS_FILE:-$GIT_DIR/.devcontainer/signers}"src/partcad/.devcontainer/devcontainer.json (1)
16-21: Consider upgrading Python versionThe configuration uses Python 3.10. Consider upgrading to Python 3.11 or 3.12 for better performance and newer features.
- "version": "3.10" + "version": "3.12"README.md (3)
15-18: Format the URL as a proper markdown link.Convert the bare URL to a proper markdown link for better readability and maintainability.
-* https://mcr.microsoft.com/en-us/artifact/mar/devcontainers/base/tags +* [Microsoft Container Registry - Base Dev Container Images](https://mcr.microsoft.com/en-us/artifact/mar/devcontainers/base/tags)🧰 Tools
🪛 Markdownlint (0.37.0)
17-17: null
Bare URL used(MD034, no-bare-urls)
29-32: Consider removing debug logging flags from example commands.The example commands include
--log-level debug/traceflags which are typically used for troubleshooting. Consider removing these flags from the documentation unless they serve a specific purpose for users following these instructions.-devcontainer templates apply --template-id ghcr.io/partcad/devcontainers-templates/ubik --workspace-folder tmp --log-level debug -devcontainer build --push --log-level debug --image-name test --label test --workspace-folder ./tmp/ +devcontainer templates apply --template-id ghcr.io/partcad/devcontainers-templates/ubik --workspace-folder tmp +devcontainer build --push --image-name test --label test --workspace-folder ./tmp/ -devcontainer templates apply --template-id ghcr.io/partcad/devcontainers-templates/partcad --workspace-folder build/partcad --log-level trace -devcontainer build --push --log-level debug --image-name ghcr.io/partcad/devcontainer-partcad:latest --workspace-folder build/partcad # --label test +devcontainer templates apply --template-id ghcr.io/partcad/devcontainers-templates/partcad --workspace-folder build/partcad +devcontainer build --push --image-name ghcr.io/partcad/devcontainer-partcad:latest --workspace-folder build/partcadAlso applies to: 36-39
46-48: Enhance the TODO section with more context and proper link formatting.The TODO section could be more informative:
- Add context about why this guide needs to be added
- Format the URL as a proper markdown link
# TODO -Add https://github.com/devcontainers/template-starter?tab=readme-ov-file#dev-container-templates-self-authoring-guide +Add a reference to the [Dev Container Templates Self-Authoring Guide](https://github.com/devcontainers/template-starter?tab=readme-ov-file#dev-container-templates-self-authoring-guide) to help users create their own templates.🧰 Tools
🪛 Markdownlint (0.37.0)
48-48: null
Bare URL used(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.devcontainer/devcontainer-lock.json(1 hunks).devcontainer/devcontainer.json(1 hunks).github/actions/smoke-test/build.sh(1 hunks).github/workflows/release.yaml(2 hunks).github/workflows/test-pr.yaml(2 hunks).gitignore(1 hunks).vscode/settings.json(1 hunks)README.md(1 hunks)src/partcad/.devcontainer/Dockerfile(1 hunks)src/partcad/.devcontainer/apt-compile.sh(1 hunks)src/partcad/.devcontainer/apt.in(1 hunks)src/partcad/.devcontainer/apt.sh(1 hunks)src/partcad/.devcontainer/apt.txt(1 hunks)src/partcad/.devcontainer/authorized_keys(1 hunks)src/partcad/.devcontainer/bootstrap.sh(1 hunks)src/partcad/.devcontainer/devcontainer-lock.json(1 hunks)src/partcad/.devcontainer/devcontainer.json(1 hunks)src/partcad/.devcontainer/ssh-sign.sh(1 hunks)src/partcad/NOTES.md(1 hunks)src/partcad/devcontainer-template.json(1 hunks)src/ubik/.devcontainer/devcontainer.json(1 hunks)src/ubik/devcontainer-template.json(1 hunks)test/partcad/test.sh(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- .gitignore
- src/partcad/NOTES.md
- src/partcad/.devcontainer/authorized_keys
- src/partcad/devcontainer-template.json
- src/partcad/.devcontainer/apt.sh
- src/ubik/devcontainer-template.json
- src/partcad/.devcontainer/devcontainer-lock.json
- src/ubik/.devcontainer/devcontainer.json
- src/partcad/.devcontainer/apt.in
- src/partcad/.devcontainer/apt.txt
🧰 Additional context used
🪛 Shellcheck (0.10.0)
test/partcad/test.sh
[warning] 2-2: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 2-2: Quote this to prevent word splitting.
(SC2046)
🪛 LanguageTool
README.md
[uncategorized] ~6-~6: Possible missing comma found.
Context: ...aml) ## Adding Templates to the Index Next you will need to add your Templates col...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 Markdownlint (0.37.0)
README.md
17-17: null
Bare URL used
(MD034, no-bare-urls)
48-48: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (9)
.github/workflows/test-pr.yaml (1)
16-16: LGTM: Path filter for PartCAD templates
The addition of the partcad filter follows the existing pattern and will properly trigger tests for changes in the PartCAD-related files.
.github/workflows/release.yaml (1)
8-8: LGTM: Consistent Ubuntu version update
The update to ubuntu-24.04 maintains consistency with other workflows and ensures reproducible builds.
.vscode/settings.json (1)
16-22: LGTM! Custom dictionary entries are valid.
The added spell checker words correctly include project-specific terms and tool names that are used across the codebase.
src/partcad/.devcontainer/Dockerfile (2)
6-14: LGTM! Well-documented OCI labels.
The labels section is well-structured and follows the OpenContainers specification with proper documentation links.
18-21: Verify apt.sh and apt.txt availability.
The Dockerfile mounts apt.sh and apt.txt, but we need to ensure these files exist and are properly configured.
✅ Verification successful
Let me check the content of these files to ensure they are properly configured.
Both apt.sh and apt.txt are present and properly configured
The verification shows that:
- Both files exist in the correct location
- apt.sh is executable and contains proper shell script with error handling
- apt.txt contains a well-formatted list of packages with specific versions
- The script correctly uses the mounted apt.txt file at /tmp/apt.txt
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if required files exist and are executable
for file in "src/partcad/.devcontainer/apt.sh" "src/partcad/.devcontainer/apt.txt"; do
if [ -f "$file" ]; then
echo "$file exists"
if [[ "$file" == *.sh ]]; then
if [ -x "$file" ]; then
echo "$file is executable"
else
echo "$file is not executable"
fi
fi
else
echo "$file not found"
fi
done
Length of output: 690
Script:
#!/bin/bash
# Check content of apt.sh and apt.txt
echo "=== Content of apt.sh ==="
cat src/partcad/.devcontainer/apt.sh
echo -e "\n=== Content of apt.txt ==="
cat src/partcad/.devcontainer/apt.txt
Length of output: 3027
.devcontainer/devcontainer-lock.json (1)
23-30: LGTM! Well-structured feature configuration.
The pip feature is properly configured with:
- Pinned version
- Integrity check
- Appropriate dependency on the apt feature
.devcontainer/devcontainer.json (1)
4-8: Verify environment variable security implications
The WAKATIME_API_KEY and GITHUB_TOKEN are being passed from local environment to container. While this is a common practice, ensure these tokens have appropriate scope limitations.
✅ Verification successful
Environment variables appear to be used securely
The WAKATIME_API_KEY is only used locally in the devcontainer and isn't referenced elsewhere in the codebase. The GITHUB_TOKEN is only used in GitHub Actions workflows with appropriate permission scopes (contents, pull-requests, packages) and is properly sourced from GitHub's secrets. The environment variable configuration in the devcontainer.json is following secure practices by:
- Using local environment variable references (
${localEnv:}) - Not exposing tokens in plain text
- Limiting token usage to the development container context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these environment variables are referenced in any documentation or CI files
rg -l "WAKATIME_API_KEY|GITHUB_TOKEN" --type md --type yaml
Length of output: 61
Script:
#!/bin/bash
# Let's check for any usage or configuration of these tokens in the codebase
rg -i "WAKATIME_API_KEY|GITHUB_TOKEN" -A 3 -B 3
# Also check for any GitHub Actions workflow files specifically
fd -e yml -e yaml . .github/
Length of output: 181
Script:
#!/bin/bash
# Let's examine the GitHub Actions workflows to see how these tokens are used
cat .github/workflows/release.yaml .github/workflows/test-pr.yaml .github/actions/smoke-test/action.yaml
# Let's also check for any configuration files that might define token scopes or permissions
fd -e json -e jsonc . | xargs cat
Length of output: 5798
src/partcad/.devcontainer/devcontainer.json (1)
77-77: Uncomment and resolve the YAML extension conflict
The YAML extension is commented out due to a command conflict. This should be resolved as YAML support is important for configuration files.
Would you like me to help investigate the command conflict and propose a solution?
✅ Verification successful
YAML extension can be safely uncommented
The repository contains YAML files (.pre-commit-config.yaml), and while there's a comment about a command conflict with jumpToSchema, no actual conflicts were found in the current codebase. The YAML extension can be uncommented to provide better YAML support, especially for configuration files.
- Uncomment the line:
"redhat.vscode-yaml" - Remove the TODO comment about the command conflict
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for YAML files in the repository
fd -e yaml -e yml
Length of output: 19
Script:
#!/bin/bash
# Check for YAML files using ripgrep which can also search within .devcontainer directories
rg --type yaml "" -l
# Also check for any other extensions that might be causing the conflict
rg "jumpToSchema" -A 2 -B 2
Length of output: 74
README.md (1)
5-14: LGTM! Improved document structure.
The header level changes create a more logical document hierarchy.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: Possible missing comma found.
Context: ...aml) ## Adding Templates to the Index Next you will need to add your Templates col...
(AI_HYDRA_LEO_MISSING_COMMA)
| # - name: Create PR for Documentation | ||
| # id: push_image_info | ||
| # env: | ||
| # GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| # run: | | ||
| # set -e | ||
| # echo "Start." | ||
|
|
||
| # # Configure git and Push updates | ||
| # git config --global user.email github-actions[bot]@users.noreply.github.com | ||
| # git config --global user.name github-actions[bot] | ||
| # git config pull.rebase false | ||
|
|
||
| # branch=automated-documentation-update-$GITHUB_RUN_ID | ||
| # git checkout -b $branch | ||
| # message='Automated documentation update' | ||
|
|
||
| # # Add / update and commit | ||
| # git add */**/README.md | ||
| # git commit -m 'Automated documentation update [skip ci]' || export NO_UPDATES=true | ||
|
|
||
| # # Push | ||
| # if [ "$NO_UPDATES" != "true" ] ; then | ||
| # git push origin "$branch" | ||
| # gh pr create --title "$message" --body "$message" | ||
| # fi |
There was a problem hiding this comment.
💡 Codebase verification
Remove commented out documentation update workflow code
The commented-out code block for automated documentation updates appears to be the only implementation of this functionality in the repository, and there are no active alternatives or related issues tracking its restoration. Since this code involves repository write permissions and automated PR creation, it's crucial to maintain a clean and unambiguous CI/CD configuration.
- Remove the entire commented block (lines 26-51) in
.github/workflows/release.yaml - If this functionality needs to be restored in the future, it can be retrieved from the git history
🔗 Analysis chain
Remove commented out code block
Large blocks of commented-out code should be removed from version control:
- If this functionality is temporarily disabled, create an issue to track its restoration
- If it's permanently removed, delete it entirely
- The code history will preserve this functionality if needed later
This is particularly important for CI/CD code that handles repository write permissions.
Let's check if this documentation update functionality is tracked elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related issues or alternative implementation
echo "Checking for related issues..."
gh issue list --search "documentation update workflow"
echo "Checking for alternative documentation workflow files..."
fd -e yaml -e yml . .github/workflows/ --exec grep -l "documentation"
Length of output: 351
Script:
#!/bin/bash
# Let's check git history to see when and why this code was commented out
git log -p -- .github/workflows/release.yaml | grep -A 10 -B 10 "Create PR for Documentation"
# Let's also check if there are any other files that might be handling documentation updates
rg -l "documentation" --type yaml
rg -l "README.md" --type yaml
# Check if there are any active workflows that modify or commit files
rg "git (add|commit|push)" .github/workflows/
Length of output: 376
| echo "Configuring Git safe directory: ${WORKSPACE_DIR}" | ||
| if ! git config --global --add safe.directory "${WORKSPACE_DIR}"; then | ||
| echo "Failed to configure Git safe directory. Ensure Git is installed and you have proper permissions." | ||
| exit 1 | ||
| fi | ||
|
|
||
| cd "${WORKSPACE_DIR}" || { echo "Failed to change directory to ${WORKSPACE_DIR}. Please verify the directory exists."; exit 1; } | ||
| if ! git config --global --add safe.directory "${WORKSPACE_DIR}"; then | ||
| echo "Failed to configure Git safe directory" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate Git safe directory configuration.
The Git safe directory configuration is duplicated unnecessarily. The second block (lines 23-26) is redundant as it performs the exact same operation as the first block.
echo "Configuring Git safe directory: ${WORKSPACE_DIR}"
if ! git config --global --add safe.directory "${WORKSPACE_DIR}"; then
echo "Failed to configure Git safe directory. Ensure Git is installed and you have proper permissions."
exit 1
fi
cd "${WORKSPACE_DIR}" || { echo "Failed to change directory to ${WORKSPACE_DIR}. Please verify the directory exists."; exit 1; }
-if ! git config --global --add safe.directory "${WORKSPACE_DIR}"; then
- echo "Failed to configure Git safe directory"
- exit 1
-fi📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Configuring Git safe directory: ${WORKSPACE_DIR}" | |
| if ! git config --global --add safe.directory "${WORKSPACE_DIR}"; then | |
| echo "Failed to configure Git safe directory. Ensure Git is installed and you have proper permissions." | |
| exit 1 | |
| fi | |
| cd "${WORKSPACE_DIR}" || { echo "Failed to change directory to ${WORKSPACE_DIR}. Please verify the directory exists."; exit 1; } | |
| if ! git config --global --add safe.directory "${WORKSPACE_DIR}"; then | |
| echo "Failed to configure Git safe directory" | |
| exit 1 | |
| fi | |
| echo "Configuring Git safe directory: ${WORKSPACE_DIR}" | |
| if ! git config --global --add safe.directory "${WORKSPACE_DIR}"; then | |
| echo "Failed to configure Git safe directory. Ensure Git is installed and you have proper permissions." | |
| exit 1 | |
| fi | |
| cd "${WORKSPACE_DIR}" || { echo "Failed to change directory to ${WORKSPACE_DIR}. Please verify the directory exists."; exit 1; } |
| # Install Git LFS | ||
| echo "Installing Git LFS..." | ||
| if ! curl -s https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | sudo bash; then | ||
| echo "Failed to add Git LFS repository" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Enhance security for Git LFS installation.
Executing scripts directly from URLs poses security risks. Consider:
- Verifying the script's checksum before execution
- Using the package manager's native Git LFS package instead
-# Install Git LFS
-echo "Installing Git LFS..."
-if ! curl -s https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | sudo bash; then
- echo "Failed to add Git LFS repository"
- exit 1
-fi
+# Install Git LFS using package manager
+echo "Installing Git LFS..."
+if ! sudo apt-get update && sudo apt-get install -y git-lfs; then
+ echo "Failed to install Git LFS"
+ exit 1
+fi📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Install Git LFS | |
| echo "Installing Git LFS..." | |
| if ! curl -s https://packagecloud.io/install/repositories/github/git-lfs/script.deb.sh | sudo bash; then | |
| echo "Failed to add Git LFS repository" | |
| exit 1 | |
| fi | |
| # Install Git LFS using package manager | |
| echo "Installing Git LFS..." | |
| if ! sudo apt-get update && sudo apt-get install -y git-lfs; then | |
| echo "Failed to install Git LFS" | |
| exit 1 | |
| fi |
|
|
||
| - Workspace: ${WORKSPACE_DIR} | ||
| - Allure: $(allure --version) | ||
| - Pre-commit: $(pre-commit --version) |
There was a problem hiding this comment.
Add missing pre-commit installation.
The summary displays pre-commit version (pre-commit --version), but the installation of pre-commit is not visible in the script.
Add the following before the summary section:
+# Install pre-commit
+echo "Installing pre-commit..."
+if ! pip install pre-commit; then
+ echo "Failed to install pre-commit"
+ exit 1
+fi📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Pre-commit: $(pre-commit --version) | |
| # Install pre-commit | |
| echo "Installing pre-commit..." | |
| if ! pip install pre-commit; then | |
| echo "Failed to install pre-commit" | |
| exit 1 | |
| fi | |
| - Pre-commit: $(pre-commit --version) |
| install_component() { | ||
| local component="$1" | ||
| local command="$2" | ||
|
|
||
| # Input validation | ||
| if [[ -z "${component}" || -z "${command}" ]]; then | ||
| echo "Error: Component name and command are required" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Installing ${component}..." | ||
| # shellcheck disable=SC2068 | ||
| if ! ${command[@]}; then | ||
| echo "Failed to install ${component}" | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve component installation error handling and array usage.
The install_component function has a few areas for improvement:
- Using
exitwithin a function makes it non-reusable and harder to handle errors at the caller level - The shellcheck disable for SC2068 suggests improper array handling
install_component() {
local component="$1"
local command="$2"
+ local status=0
# Input validation
if [[ -z "${component}" || -z "${command}" ]]; then
echo "Error: Component name and command are required"
- exit 1
+ return 1
fi
echo "Installing ${component}..."
- # shellcheck disable=SC2068
- if ! ${command[@]}; then
+ if ! eval "${command}"; then
echo "Failed to install ${component}"
- exit 1
+ return 1
fi
+ return 0
}Committable suggestion skipped: line range outside the PR's diff.
| "streetsidesoftware.code-spell-checker", | ||
| "streetsidesoftware.code-spell-checker", |
There was a problem hiding this comment.
Remove duplicate extension entry
The extension streetsidesoftware.code-spell-checker is listed twice in the extensions array.
- "streetsidesoftware.code-spell-checker",📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "streetsidesoftware.code-spell-checker", | |
| "streetsidesoftware.code-spell-checker", | |
| "streetsidesoftware.code-spell-checker", |
| "ghcr.io/partcad/devcontainers-features/apt:2": { | ||
| "packages": "bash-completion shellcheck mc python3-pip python3-venv" // shfmt | ||
| }, | ||
| "ghcr.io/partcad/devcontainers-features/pip:2": { | ||
| "packages": "pre-commit mkdocs mkdocs-material", | ||
| "location": "/opt/pip" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider version pinning for pip packages
The pip feature installs packages without version constraints. This could lead to compatibility issues if package versions change.
- "packages": "pre-commit mkdocs mkdocs-material",
+ "packages": "pre-commit==3.5.0 mkdocs==1.5.3 mkdocs-material==9.4.14",📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "ghcr.io/partcad/devcontainers-features/apt:2": { | |
| "packages": "bash-completion shellcheck mc python3-pip python3-venv" // shfmt | |
| }, | |
| "ghcr.io/partcad/devcontainers-features/pip:2": { | |
| "packages": "pre-commit mkdocs mkdocs-material", | |
| "location": "/opt/pip" | |
| } | |
| "ghcr.io/partcad/devcontainers-features/apt:2": { | |
| "packages": "bash-completion shellcheck mc python3-pip python3-venv" // shfmt | |
| }, | |
| "ghcr.io/partcad/devcontainers-features/pip:2": { | |
| "packages": "pre-commit==3.5.0 mkdocs==1.5.3 mkdocs-material==9.4.14", | |
| "location": "/opt/pip" | |
| } |
| # TODO: alexanderilyin: check that it is set | ||
| USER_EMAIL=$(git config user.email) |
There was a problem hiding this comment.
Remove TODO comment and implement the check
The TODO comment suggests checking if the email is set, but the check is already implemented below.
-# TODO: alexanderilyin: check that it is set
USER_EMAIL=$(git config user.email)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # TODO: alexanderilyin: check that it is set | |
| USER_EMAIL=$(git config user.email) | |
| USER_EMAIL=$(git config user.email) |
| if ! grep -q "$USER_EMAIL" "$SIGNERS_FILE"; then | ||
| echo "$USER_EMAIL $SELECTED_FP" >> "$SIGNERS_FILE" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for existing key entries
The script appends new entries without checking if the same key (not just email) already exists, which could lead to duplicate keys with different emails.
- if ! grep -q "$USER_EMAIL" "$SIGNERS_FILE"; then
+ if ! grep -q "^$USER_EMAIL $SELECTED_FP$" "$SIGNERS_FILE"; then
echo "$USER_EMAIL $SELECTED_FP" >> "$SIGNERS_FILE"
fi📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! grep -q "$USER_EMAIL" "$SIGNERS_FILE"; then | |
| echo "$USER_EMAIL $SELECTED_FP" >> "$SIGNERS_FILE" | |
| fi | |
| if ! grep -q "^$USER_EMAIL $SELECTED_FP$" "$SIGNERS_FILE"; then | |
| echo "$USER_EMAIL $SELECTED_FP" >> "$SIGNERS_FILE" | |
| fi |
| "version": "3.10" | ||
| }, | ||
| "ghcr.io/devcontainers/features/conda:1": { | ||
| "version": "24.5.0" // GH has 24.11.1 |
There was a problem hiding this comment.
Address TODO comments with concrete actions
There are two TODO comments that should be addressed:
- Conda version mismatch note
- Moving postCreateCommand to Dockerfile
For the Conda version:
- "version": "24.5.0" // GH has 24.11.1
+ "version": "24.11.1"For the postCreateCommand, create a new issue to track the optimization task.
Would you like me to create a GitHub issue to track moving the bootstrap command to the Dockerfile?
Also applies to: 41-41
…tainer-partcad.tgz does not exist.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Bug Fixes