Fix log level and error handling #27
Conversation
WalkthroughThis pull request updates the logging behavior in Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lua/devcontainer-cli/log.lua(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit-tests
- GitHub Check: unit-tests
🔇 Additional comments (8)
lua/devcontainer-cli/log.lua (8)
14-14: Consistent string formatting applied.Standardizing to double quotes for strings improves consistency throughout the codebase.
32-37: Cleaned up table format by removing trailing commas.Removing trailing commas in the modes table entries is a good cleanup that maintains Lua syntax correctness while improving code style consistency.
52-52: Consistent string formatting in outfile path.Changing to double quotes for string formatting maintains consistency with other string formatting in the file.
69-69: Minor format improvement to the round function.The formatting change to the rounding logic improves readability without changing the functionality.
74-74: Improved select function call formatting.Minor syntax enhancement for better readability in the loop iteration over variadic arguments.
123-131: Enhanced error handling for log file operations.Excellent improvement by wrapping the file operations in
pcalland adding error notifications when writing to the log file fails. This prevents potential silent failures when log files can't be created or written to.
140-140: Improved string formatting syntax for dynamic function names.The change to use the
formatmethod directly with parentheses is a cleaner approach.
142-142: Simplified variable initialization for variadic arguments.Creating the
passedvariable with a cleaner syntax improves code readability.
|
Caution No docstrings were generated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
2-2: Maintain Consistent Comment Formatting
The standalone comment on line 2 appears to have been modified. If its purpose is solely for visual separation, consider either enhancing its content for clarity or removing it to avoid unnecessary clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unit-tests
- GitHub Check: unit-tests
🔇 Additional comments (5)
Dockerfile (5)
23-38: Streamlined Dependency Installation
The consolidatedapt-getcommand updates and installs all necessary dependencies in one go, and the inclusion of cleanup commands (apt-get autoremove -yand removal of/var/lib/apt/lists/*) is an excellent measure for reducing the final image size. Please verify that these dependencies fully support the updated logging features (e.g., Lua testing) introduced in the related log-handling changes.
40-41: Working Directory Setup
TheWORKDIR /appdirective is appropriately placed after the dependency installation stage. It clearly defines the context for subsequent operations.
42-44: Devcontainers CLI Installation
The command to install the devcontainers CLI (RUN npm install -g @devcontainers/[email protected]) is clear and reproducible by pinning the CLI version. Ensure that version0.49.0aligns with the overall project requirements and logging improvements.
45-47: Lua Dependencies for Testing
Installing testing dependencies vialuarocks install bustedis a straightforward and effective change that supports Lua project testing. Should more Lua packages become necessary with further log system enhancements, consider grouping related Lua dependencies for easier maintenance.
59-59: Secure Sudoers File Permission Setting
The modification on line 59 ensures that the sudoers file for the user has the correct permissions (0440), which is crucial for maintaining system security. This is a best practice, given the file’s sensitivity.
erichlf
force-pushed
the
logger_fixes
branch
2 times, most recently
from
ff9c53c to
0686bf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/minimal_init.lua (3)
21-27: Add a docstring or comment clarifying the purpose ofensure_dir().
Currently, this function is self-explanatory but adding a short docstring would help future maintainers.+--- Creates the directory at the specified path if it does not already exist. local function ensure_dir(path) local stat = vim.loop.fs_stat(path) if not stat then vim.fn.mkdir(path, "p") end end
33-42: Consider consolidating thelogandstatefallback logic.
The approach forlog_pathcan be reused forstate_path, or both can be handled in a single helper function to reduce duplication. This is optional but can simplify maintenance.
44-51: Same fallback strategy as above.
You might unify the logic forlogandstateby creating a small wrapper function that callsvim.fn.stdpath("...")and falls back to"data/..."if unsupported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/minimal_init.lua(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unit-tests
🔇 Additional comments (2)
tests/minimal_init.lua (2)
2-19: License text formatting update.
No functional changes identified.
29-32: Verify that directories are created correctly.
It may be helpful to confirm these directories are successfully created during test setup, for example, by asserting filesystem existence.Would you like me to provide a script or sample test code snippet to verify directory creation for these standard paths?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/default.yml (1)
14-15: Update Checkout Action Version
Static analysis tools flagged that the runner foractions/checkout@v3might be outdated. Althoughv3is widely used, consider updating to a more recent minor version (if available) to benefit from the latest fixes and improvements.🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/default.yml(1 hunks)Dockerfile(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/default.yml
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: default
Dockerfile
[error] 28-28: gzip: stdin: not in gzip format. tar: Child returned status 1. tar: Error is not recoverable: exiting now.
[error] 28-28: Command failed: docker buildx build --load --build-arg BUILDKIT_INLINE_CACHE=1 -f /tmp/devcontainercli-runner/container-features/0.74.0-1741179673396/Dockerfile-with-features -t nvim-devcontainer-cli-0.10.0:latest --target dev_containers_target_stage --cache-from nvim-devcontainer-cli-0.10.0:latest --cache-from nvim-devcontainer-cli-0.10.0 --build-arg USER_ID=1000 --build-arg GROUP_ID=1000 --build-context dev_containers_feature_content_source=/tmp/devcontainercli-runner/container-features/0.74.0-1741179673396 --build-arg _DEV_CONTAINERS_BASE_IMAGE=builder --build-arg _DEV_CONTAINERS_IMAGE_USER=my-app --build-arg _DEV_CONTAINERS_FEATURE_CONTENT_SOURCE=dev_container_feature_content_temp.
🔇 Additional comments (5)
Dockerfile (1)
21-21: New Build Argument Added
The new build argumentNEOVIM_VERSION=0.10.0is properly defined to allow dynamic version selection for Neovim. Make sure downstream commands and documentation are updated accordingly..github/workflows/default.yml (4)
1-1: Simplified Workflow Trigger
The trigger has been updated toon: [pull_request], which aligns with the revised CI/CD strategy for this PR.
8-13: Matrix Strategy for Multiple Neovim Versions
The introduction of a matrix strategy withneovim-version: ["0.9.5", "0.10.0"]andfail-fast: falseensures that tests run across multiple Neovim versions, thereby increasing the robustness of the CI process.
16-23: Consistent Pre-build Image Configuration
The “Pre-build devcontainer image” step correctly applies theNEOVIM_VERSIONvia build arguments and properly adjusts theimageNameandcacheFromfields. This change aligns well with the Dockerfile modifications.
24-29: Test Execution Step Verification
The final step runs tests inside the Docker image, and the cache is appropriately scoped to the Neovim version. This ensures that the tests accurately reflect the runtime environment.
erichlf
force-pushed
the
logger_fixes
branch
2 times, most recently
from
fbeffb7 to
b7d8851
Compare
erichlf
force-pushed
the
logger_fixes
branch
4 times, most recently
from
5f0d66b to
ab29342
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/default.yml (1)
15-15: Update checkout action to a newer version.The current checkout action version (v3) is flagged as outdated by static analysis. GitHub regularly updates actions with security improvements and bug fixes.
- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/default.yml(1 hunks)Dockerfile(3 hunks)README.md(2 hunks)tests/minimal_init.lua(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/default.yml
15-15: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (6)
tests/minimal_init.lua (2)
20-21: Proper runtime setup improves plugin test environment.Adding these lines properly configures the Neovim runtime environment by ensuring the standard data path is included in the runtime path and setting the packpath to match. This is a best practice that helps ensure plugins are correctly loaded during testing.
25-26: Improved git clone efficiency with depth parameter.Using
--depth=1when cloning Plenary is a good optimization as it only fetches the latest snapshot instead of the entire repository history, making the test setup faster and more efficient..github/workflows/default.yml (4)
1-1: Verify the impact of removing the push trigger.Limiting workflow execution to only pull requests and removing the push trigger means the tests won't run when changes are pushed directly to important branches (like main/master). Consider whether this is the intended behavior for this repository's workflow.
8-11: Great addition of Neovim version matrix.Testing against multiple Neovim versions (0.9.5 and 0.10.0) with the matrix strategy ensures broader compatibility. Setting
fail-fast: falseis also good practice as it allows all version tests to complete even if one fails.
19-23: Well-structured Docker image configuration with version parameterization.Using the matrix Neovim version to create distinct Docker images with appropriate caching and build arguments is a good approach. This ensures each test environment precisely matches its target Neovim version.
27-27: Consistent cache usage across workflow steps.Using the versioned image name for caching ensures the right environment is used for tests, maintaining consistency with the pre-build step.
erichlf
force-pushed
the
logger_fixes
branch
4 times, most recently
from
a225814 to
a845007
Compare
erichlf
force-pushed
the
logger_fixes
branch
2 times, most recently
from
4c11fbd to
18a5a34
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Dockerfile (1)
45-53: 🛠️ Refactor suggestion
⚠️ Potential issueShell Conditional and Extraction Logic Issues.
The RUN command block uses the bash-specific[[ ... ]]construct which may fail under/bin/sh(typically dash in Ubuntu). Please update it to a POSIX-compliant conditional, for example:- if [[ "$NEOVIM_VERSION" == "stable" ]]; then \ + if [ "$NEOVIM_VERSION" = "stable" ]; then \Additionally, the tar extraction and symlink creation steps assume that the extracted directory name will match the pattern used for the symlink. This could lead to a dangling symlink if the tarball’s internal directory name (e.g.
nvim-linux-x86_64ornvim-linux64) does not conform to the expected pattern.
Consider refactoring this segment to first extract the tarball without a target directory (or extract it in the current working directory), then rename/move the extracted folder to a fixed location (e.g./opt/nvim) before creating the symlink. This approach was detailed in previous review comments and remains applicable.🧰 Tools
🪛 GitHub Actions: default
[error] 46-46: /bin/sh: 1: [[: not found
[error] 46-46: curl: (22) The requested URL returned error: 404
[error] 46-46: Command failed: docker buildx build --load --build-arg BUILDKIT_INLINE_CACHE=1 -f /tmp/devcontainercli-runner/container-features/0.74.0-1741190163596/Dockerfile-with-features -t nvim-devcontainer-cli-v0.9.1:latest --target dev_containers_target_stage --cache-from nvim-devcontainer-cli-v0.9.1:latest --cache-from nvim-devcontainer-cli-v0.9.1 --build-arg USER_ID=1000 --build-arg GROUP_ID=1000 --build-context dev_containers_feature_content_source=/tmp/devcontainercli-runner/container-features/0.74.0-1741190163596 --build-arg _DEV_CONTAINERS_BASE_IMAGE=builder --build-arg _DEV_CONTAINERS_IMAGE_USER=my-app --build-arg _DEV_CONTAINERS_FEATURE_CONTENT_SOURCE=dev_container_feature_content_temp /home/runner/work/devcontainer-cli.nvim/devcontainer-cli.nvim (exit code: undefined)
🧹 Nitpick comments (1)
.github/workflows/default.yml (1)
43-43: Note on Log Level and Error Handling Changes.
The PR title indicates improvements to log level and error handling. However, none of the changes in this file address logging functionality. Please ensure that the intended changes inlua/devcontainer-cli/log.lua(or the relevant module) are included and reviewed separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/default.yml(1 hunks)Dockerfile(3 hunks)README.md(2 hunks)tests/minimal_init.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- tests/minimal_init.lua
🧰 Additional context used
🪛 GitHub Actions: default
Dockerfile
[error] 46-46: /bin/sh: 1: [[: not found
[error] 46-46: curl: (22) The requested URL returned error: 404
[error] 46-46: Command failed: docker buildx build --load --build-arg BUILDKIT_INLINE_CACHE=1 -f /tmp/devcontainercli-runner/container-features/0.74.0-1741190163596/Dockerfile-with-features -t nvim-devcontainer-cli-v0.9.1:latest --target dev_containers_target_stage --cache-from nvim-devcontainer-cli-v0.9.1:latest --cache-from nvim-devcontainer-cli-v0.9.1 --build-arg USER_ID=1000 --build-arg GROUP_ID=1000 --build-context dev_containers_feature_content_source=/tmp/devcontainercli-runner/container-features/0.74.0-1741190163596 --build-arg _DEV_CONTAINERS_BASE_IMAGE=builder --build-arg _DEV_CONTAINERS_IMAGE_USER=my-app --build-arg _DEV_CONTAINERS_FEATURE_CONTENT_SOURCE=dev_container_feature_content_temp /home/runner/work/devcontainer-cli.nvim/devcontainer-cli.nvim (exit code: undefined)
🔇 Additional comments (6)
Dockerfile (4)
22-23: NEOVIM_VERSION argument defined correctly.
The declaration ofARG NEOVIM_VERSION="stable"is clear and sets a default version for Neovim, enabling dynamic builds based on the version supplied.
25-40: Dependency Installation Steps.
The updated RUN command that installs dependencies withoutsudoand clears the apt cache is well-structured. This optimization reduces bloat in the final image.
55-61: Devcontainer CLI and Lua Dependency Installation.
The process for installing the devcontainers CLI and Lua dependencies (using npm and luarocks) is clear and consistent.
66-73: User and Sudoers Setup.
The user creation, group assignment, and sudoers configuration are correctly implemented. The explicit permission setup withchmod 0440contributes to system security..github/workflows/default.yml (2)
1-24: Workflow Trigger and Matrix Strategy Update.
Switching the trigger topull_requestand introducing a matrix strategy for running tests with multiple Neovim versions enhances testing coverage and ensures compatibility with various versions. The YAML structure is clean and effective.
25-42: CI Steps and Pre-build Configuration.
The checkout step is updated to useactions/checkout@v4, and the pre-build step properly passes the Neovim version as a build argument. The use of environment variables to configureimageNameandcacheFromensures consistency with the Dockerfile changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Dockerfile (3)
45-49: Conditional Download of Neovim Tarball
Using anifstatement to download different tarballs based on theNEOVIM_VERSIONis a practical solution. However, consider verifying that the filenames (nvim-linux-x86_64.tar.gzvs.nvim-linux64.tar.gz) exactly match the release asset names on GitHub. Additionally, it may be beneficial to add error checking (for example, ensuring thecurlcommand succeeds) so that failures in downloading are caught early.
50-53: Tar Extraction and Post-Extraction Cleanup
The extraction command using the wildcard (nvim-*.tar.gz) and the subsequent creation of a symbolic link works fine. As an enhancement, consider cleaning up the downloaded tarball after extraction to reduce the image size and remove temporary files.
67-72: User Setup and Sudo Installation
The commands to add a group, add a user, installsudo, and set the sudoers permissions are standard. Given that one of the PR objectives was to remove unnecessary usage ofsudo, please double-check whether installingsudois still required. If your workflows run as root or if elevated privileges aren’t needed post-build, removingsudomay simplify your image and tighten security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/default.yml(1 hunks)Dockerfile(3 hunks)README.md(2 hunks)tests/minimal_init.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- tests/minimal_init.lua
🔇 Additional comments (6)
Dockerfile (1)
22-23: Add NEOVIM_VERSION Build Argument
Adding the build argument with a default value of"stable"is a clear and useful addition. This lets you control the Neovim version at build time. Please ensure that all references to this argument in subsequent commands remain consistent..github/workflows/default.yml (5)
1-1: Pull Request Trigger Configuration
Switching the event trigger topull_requestaligns well with the current CI strategy. This ensures that the tests run only when necessary.
8-24: Matrix Strategy for Neovim Versions
The matrix strategy is well defined. Testing across multiple Neovim versions—including explicit versions and"stable"—ensures broad compatibility. This setup helps catch issues across different environments.
28-29: Update Checkout Action Version
Usingactions/checkout@v4is a positive update. It keeps your workflow up to date with the latest improvements and fixes from the checkout action.
32-35: Pre-build Step and Build Arguments
The pre-build step correctly leverages thedevcontainers/[email protected]action. The image naming and caching strategy are clear, and the build argument is formatted as a space-separated key-value pair, which resolves previous formatting issues.
37-41: Running Tests in the Docker Image
Runningmake testinside the built image is straightforward. Just verify that all needed test dependencies are present in the image so that tests run reliably across the different Neovim versions.
erichlf
force-pushed
the
logger_fixes
branch
4 times, most recently
from
439f322 to
23bd427
Compare
erichlf
force-pushed
the
logger_fixes
branch
8 times, most recently
from
5bbe79f to
fdd4030
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile (2)
27-41: Dependency Installation and Cleanup
The updated installation block (lines 27–41) effectively installs the required packages without unnecessary privileges. The use ofDEBIAN_FRONTEND=noninteractiveand cleanup commands improves reliability and reduces image bloat.Optional: Consider verifying if every listed package is needed to keep the image as lean as possible.
47-55: Conditional NEOVIM Installation
The conditional block (lines 47–55) correctly selects the appropriate Neovim tarball based on whetherNEOVIM_VERSIONequals"stable". Usingcurl -fLOensures that failures stop the build.Optional: You might consider adding comments or additional error handling (if deemed necessary) to document the rationale behind the different tarball names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/default.yml(1 hunks)Dockerfile(3 hunks)README.md(2 hunks)tests/minimal_init.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- tests/minimal_init.lua
🔇 Additional comments (8)
Dockerfile (3)
21-21: Default NEOVIM_VERSION Argument Initialization
The addition ofARG NEOVIM_VERSION="stable"at line 21 correctly provides a default value and makes the build more predictable.
24-24: Redeclaring NEOVIM_VERSION in the Builder Stage
Re-declaring theARG NEOVIM_VERSIONhere is necessary for multi-stage builds so that the variable is available in the builder stage. This is a proper use of Dockerfile scoping.
73-74: Securing the Sudoers File
The command at line 74 that sets permissions on the sudoers file (chmod 0440 /etc/sudoers.d/$USER_NAME) is a good security measure, ensuring that the file is not writable by unauthorized users..github/workflows/default.yml (5)
1-1: Workflow Trigger Update
Changing the event trigger toon: [pull_request]ensures that this workflow runs only when pull requests are created or updated. This reduces unnecessary builds and aligns with the PR’s focus on improving runtimes.
8-24: Enhanced Testing Matrix with Multiple Neovim Versions
The newly defined strategy matrix (lines 8–24) now includes a robust list of Neovim versions—including"stable"—to test the devcontainer across a variety of environments. This will improve compatibility verification.
28-28: Checkout Action Upgrade
The upgrade of the checkout step toactions/checkout@v4is current and helps ensure better performance and security.
32-35: Dynamic Devcontainer Image Pre-Build
By constructing the image name and cache key with${{ env.IMAGE_NAME }}-${{ matrix.neovim-version }}and passingNEOVIM_VERSION=${{ matrix.neovim-version }}to the build step, the workflow cleanly ties the Dockerfile parameter to the testing matrix. This ensures that the correct Neovim version is installed.
39-39: Consistency in Test Step Cache Usage
The test step also uses the updated cache key format (line 39), which ensures consistency between the build and test phases.
This PR fixes some issue with log levels and adds some error handling to creating log files.
Summary by CodeRabbit