Add provider detection for more managers - #28
Conversation
|
Warning Rate limit exceeded@akriaueno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds many new provider-detection rules (version managers, MacPorts, Nix, Windows managers, multiple distro package managers), refactors system-package-manager checks into a pluggable PkgManagerStrategy list, expands README, adds extensive unit tests, and introduces Docker-based end-to-end test infrastructure and per-distro e2e scripts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/why_core.nim (1)
188-196: Consider combining the zypper/rpm and yum/rpm checks to avoid redundant rpm calls.When both
zypperandrpmexist butrpm -qffails (e.g., file not from a package), the code falls through to runrpm -qfagain at line 194. This is functionally correct but executes the same external command twice.🔎 Proposed optimization
- if ctx.findExe("zypper").len > 0 and ctx.findExe("rpm").len > 0: - let (outp, exitCode) = ctx.execCmd("rpm -qf " & quoteShell(path)) - if exitCode == 0: - return "zypper/rpm (" & outp.strip() & ")" - - if ctx.findExe("rpm").len > 0: + if ctx.findExe("rpm").len > 0: let (outp, exitCode) = ctx.execCmd("rpm -qf " & quoteShell(path)) if exitCode == 0: - return "yum/rpm (" & outp.strip() & ")" + if ctx.findExe("zypper").len > 0: + return "zypper/rpm (" & outp.strip() & ")" + else: + return "yum/rpm (" & outp.strip() & ")"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)src/why_core.nim(3 hunks)tests/test_why_core.nim(2 hunks)
🔇 Additional comments (10)
README.md (1)
80-81: Documentation accurately reflects implementation.The supported providers list is comprehensive and aligns well with the detection rules added in
src/why_core.nim. The version managers and package managers are correctly categorized.src/why_core.nim (4)
53-58: MacPorts and Nix detection patterns look correct.The paths cover standard installation locations. Minor observation: Nix patterns don't have trailing slashes unlike MacPorts (
/opt/local/), but this is acceptable since/nix/storepaths are followed by hash-based directories that won't conflict.
233-241: Theequeryparsing logic correctly handles the output format.The logic properly extracts the package name from lines like
"category/package-version (/path/to/file)"and skips status lines prefixed with*.
198-203: Theapk info -W <file>command output is documented to print only the package name, not a formatted string. The test mock at line 162 correctly returns"busybox-1.36.1-r0\n", and the code correctly processes this by taking the first line and stripping whitespace. No format mismatch exists.Likely an incorrect or invalid review comment.
93-95: Rustup and Cargo rules are correctly distinguished via real path resolution.The Rustup patterns (
".rustup","rustup/toolchains") don't include".cargo/bin", which both Rustup and Cargo use as their binary location. However, the current implementation correctly detects Rustup-managed binaries becausedetectProviderByPathchecks the real symlink target (realPath) first before checking the origin path. A Rustup-managed binary likerustcat~/.cargo/bin/rustcwill resolve to a real path containingrustup/toolchains, matching the Rustup pattern before the Cargo rule is evaluated. The test at line 124 validates this behavior.tests/test_why_core.nim (5)
79-103: Good test coverage for zypper/rpm detection.The test correctly mocks both
zypperandrpmexecutables and validates the expected provider format.
114-130: Comprehensive test coverage for version manager detection.Tests cover all newly added version managers with realistic path patterns. The test structure using a table of cases is clean and maintainable.
132-145: Good cross-platform package manager detection tests.Tests appropriately cover both Unix-style paths (MacPorts, Nix) and Windows-style paths (Scoop, Chocolatey, winget).
147-169: Verify apk test mock matches real output format.The test mock returns
"busybox-1.36.1-r0\n"directly, but ifapk info -Wreturns a format like"/bin/ls is owned by busybox-1.36.1-r0", the parsing logic and test would need adjustment.Ensure the mock output matches actual
apk info -Wbehavior on Alpine Linux.
219-241: Portage qfile test correctly validates the parsing logic.The mock output
"sys-apps/coreutils-9.2 /usr/bin/ls\n"matches the expectedqfile -qvformat, and the expected provider"portage (sys-apps/coreutils-9.2)"aligns with the parsing implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
tests/e2e/opensuse/Dockerfile (1)
14-14: Bash path inconsistency (see fedora/Dockerfile comment).This uses
/bin/bashwhile fedora/Dockerfile uses/usr/bin/bash. Consider standardizing across all E2E Dockerfiles as noted in the fedora/Dockerfile review.tests/e2e/arch/Dockerfile (1)
14-14: Bash path inconsistency (see fedora/Dockerfile comment).This uses
/usr/bin/bashwhile ubuntu/Dockerfile and opensuse/Dockerfile use/bin/bash. Consider standardizing across all E2E Dockerfiles as noted in the fedora/Dockerfile review.tests/e2e/opensuse/test.sh (1)
6-15: Extract duplicated helper (see arch/test.sh comment).Same
assert_containsduplication as flagged in arch/test.sh. Apply the same refactor to extract this helper to a shared utility file.tests/e2e/alpine/test.sh (1)
6-15: Extract duplicated helper (see arch/test.sh comment).Same
assert_containsduplication as flagged in arch/test.sh. Apply the same refactor to extract this helper to a shared utility file.tests/e2e/ubuntu/test.sh (1)
6-15: Same duplication as in other test files.This
assert_containsfunction is duplicated across multiple test files. See the comment ontests/e2e/fedora/test.shlines 6-15 for the refactoring suggestion.
🧹 Nitpick comments (3)
tests/e2e/fedora/Dockerfile (1)
14-14: Consider standardizing bash paths across all E2E Dockerfiles.This Dockerfile uses
/usr/bin/bashwhile ubuntu/Dockerfile uses/bin/bash. Both are valid, but standardizing on one path across all E2E Dockerfiles would improve consistency and reduce cognitive load. Note that arch/Dockerfile also uses/usr/bin/bashwhile opensuse/Dockerfile uses/bin/bash.tests/e2e/fedora/test.sh (1)
6-15: Consider extracting the duplicated helper to a shared file.The
assert_containsfunction is duplicated identically across five test files (alpine, arch, fedora, opensuse, ubuntu). Extracting it to a shared helper file (e.g.,tests/e2e/common.sh) would improve maintainability.Example refactor
Create
tests/e2e/common.sh:#!/usr/bin/env bash assert_contains() { local haystack="$1" local needle="$2" if ! grep -qF "$needle" <<<"$haystack"; then echo "Expected output to contain: $needle" >&2 echo "Actual output:" >&2 echo "$haystack" >&2 exit 1 fi }Then source it in each test file:
+source "$(dirname "$0")/../common.sh" + -assert_contains() { - local haystack="$1" - local needle="$2" - if ! grep -qF "$needle" <<<"$haystack"; then - echo "Expected output to contain: $needle" >&2 - echo "Actual output:" >&2 - echo "$haystack" >&2 - exit 1 - fi -}tests/e2e/run.sh (1)
7-9: Optional: Address the Shellcheck warning for more robust argument handling.Shellcheck flags line 8 for potential word splitting issues. While the current pattern works for simple cases, you could make it more robust:
Alternative approach
-if [[ $# -gt 0 ]]; then - DISTROS=($@) -fi +if [[ $# -gt 0 ]]; then + DISTROS=("$@") +fiThis properly preserves argument boundaries, though in practice both work for simple distro names.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tests/e2e/README.md(1 hunks)tests/e2e/alpine/Dockerfile(1 hunks)tests/e2e/alpine/test.sh(1 hunks)tests/e2e/arch/Dockerfile(1 hunks)tests/e2e/arch/test.sh(1 hunks)tests/e2e/fedora/Dockerfile(1 hunks)tests/e2e/fedora/test.sh(1 hunks)tests/e2e/opensuse/Dockerfile(1 hunks)tests/e2e/opensuse/test.sh(1 hunks)tests/e2e/run.sh(1 hunks)tests/e2e/ubuntu/Dockerfile(1 hunks)tests/e2e/ubuntu/test.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/fedora/test.sh (4)
tests/e2e/alpine/test.sh (1)
assert_contains(6-15)tests/e2e/arch/test.sh (1)
assert_contains(6-15)tests/e2e/opensuse/test.sh (1)
assert_contains(6-15)tests/e2e/ubuntu/test.sh (1)
assert_contains(6-15)
tests/e2e/arch/test.sh (4)
tests/e2e/alpine/test.sh (1)
assert_contains(6-15)tests/e2e/fedora/test.sh (1)
assert_contains(6-15)tests/e2e/opensuse/test.sh (1)
assert_contains(6-15)tests/e2e/ubuntu/test.sh (1)
assert_contains(6-15)
tests/e2e/alpine/test.sh (4)
tests/e2e/arch/test.sh (1)
assert_contains(6-15)tests/e2e/fedora/test.sh (1)
assert_contains(6-15)tests/e2e/opensuse/test.sh (1)
assert_contains(6-15)tests/e2e/ubuntu/test.sh (1)
assert_contains(6-15)
tests/e2e/ubuntu/test.sh (4)
tests/e2e/alpine/test.sh (1)
assert_contains(6-15)tests/e2e/arch/test.sh (1)
assert_contains(6-15)tests/e2e/fedora/test.sh (1)
assert_contains(6-15)tests/e2e/opensuse/test.sh (1)
assert_contains(6-15)
🪛 Shellcheck (0.11.0)
tests/e2e/run.sh
[warning] 8-8: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🔇 Additional comments (13)
tests/e2e/arch/test.sh (1)
17-18: LGTM! Test logic is correct.The test correctly executes the
whycommand and asserts that the output contains the expected pacman provider string.tests/e2e/opensuse/test.sh (1)
17-18: LGTM! Test logic is correct.The test correctly validates zypper/rpm provider detection for OpenSUSE.
tests/e2e/alpine/test.sh (1)
17-18: LGTM! Test logic is correct.The test correctly validates apk provider detection for Alpine Linux.
tests/e2e/ubuntu/Dockerfile (1)
1-15: LGTM! Dockerfile follows best practices.The multi-stage build pattern is correct, with appropriate cleanup of package caches and use of specific version tags. The build efficiently separates the builder and runtime stages.
tests/e2e/fedora/Dockerfile (1)
1-14: LGTM! Dockerfile is well-structured.The multi-stage build pattern is correct with appropriate package cleanup. The Fedora-specific package manager commands are properly used.
tests/e2e/opensuse/Dockerfile (1)
1-14: LGTM! Dockerfile is well-structured.The multi-stage build follows best practices with proper package cleanup using zypper. The OpenSUSE-specific commands are correctly applied.
tests/e2e/arch/Dockerfile (1)
1-14: LGTM! Dockerfile is well-structured.The multi-stage build follows best practices with Arch-specific package manager commands and proper cleanup. The use of
--noconfirmflags is appropriate for automated builds.tests/e2e/README.md (1)
19-19: No action needed. The Ubuntu test script at tests/e2e/ubuntu/test.sh validates all documented providers (apt/dpkg, MacPorts, Nix, asdf, SDKMAN!, nvm, fnm, pyenv, rbenv, rvm, Rustup, Conda, Mise, and Volta) through individual provider detection assertions. The pattern observed in other distros (testing only the primary package manager) does not apply to Ubuntu, which intentionally provides comprehensive multi-provider coverage.Likely an incorrect or invalid review comment.
tests/e2e/alpine/Dockerfile (1)
1-13: LGTM!The multi-stage Dockerfile is well-structured. The builder stage compiles the Nim binary, and the runtime stage correctly installs compatibility libraries (gcompat, libc6-compat, libstdc++) needed to run the Nim binary on Alpine.
tests/e2e/fedora/test.sh (1)
17-20: LGTM!The test correctly verifies that the Why CLI detects yum/rpm as the provider on Fedora systems.
tests/e2e/run.sh (1)
11-24: LGTM!The main loop correctly validates Dockerfile existence, builds each image, and runs the tests. Error handling is appropriate.
tests/e2e/ubuntu/test.sh (2)
17-33: LGTM!The helper functions are well-designed.
make_bincreates mock executables cleanly, andrun_and_assert_providerprovides a nice abstraction for testing multiple providers.
35-92: Excellent comprehensive provider coverage!The test suite thoroughly validates detection of multiple version managers and package managers (MacPorts, Nix, asdf, SDKMAN!, nvm, fnm, pyenv, rbenv, rvm, Rustup, Conda, Mise, Volta). The Rustup test correctly simulates the symlink pattern from cargo bin to rustup toolchain. All tests follow a consistent, maintainable pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/arch/Dockerfile (2)
1-1: Consider pinning the Nim image version for reproducibility.Using
:latestcan introduce non-deterministic builds if the upstream image changes. Pinning to a specific version (e.g.,nimlang/nim:2.0.0) ensures consistent test environments.🔎 Suggested change
-FROM nimlang/nim:latest AS builder +FROM nimlang/nim:2.0.0 AS builder
9-11: LGTM with optional optimization note.The package installation and cache cleanup are appropriate for a test container. The
-Syuflag upgrades all existing packages, which ensures the latest versions but adds build time. If reproducibility is a concern, consider using-Sy(sync only) instead of-Syu.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/e2e/alpine/Dockerfile(1 hunks)tests/e2e/arch/Dockerfile(1 hunks)tests/e2e/fedora/Dockerfile(1 hunks)tests/e2e/opensuse/Dockerfile(1 hunks)tests/e2e/ubuntu/Dockerfile(1 hunks)tests/e2e/ubuntu/test.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/fedora/Dockerfile
- tests/e2e/ubuntu/test.sh
- tests/e2e/alpine/Dockerfile
- tests/e2e/opensuse/Dockerfile
- tests/e2e/ubuntu/Dockerfile
🔇 Additional comments (1)
tests/e2e/arch/Dockerfile (1)
12-14: LGTM!The binary and test script placement follow standard conventions, and the entrypoint configuration correctly executes the Arch-specific e2e test.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/README.md (1)
19-19: Consider breaking up the ubuntu provider list for readability.Line 19 is quite long and cramped, making it harder to scan the list of providers tested on Ubuntu. Consider wrapping or formatting it into multiple lines or a more structured layout.
🔎 Proposed formatting improvement
-- ubuntu: apt/dpkg + path-based providers (asdf/SDKMAN!/nvm/fnm/pyenv/rbenv/rvm/rustup/conda/mise/volta/macports/nix) +- ubuntu: apt/dpkg + path-based providers: + - asdf, SDKMAN!, nvm, fnm, pyenv, rbenv, rvm, rustup, conda, mise, volta, macports, nixAlternatively, use a table or bullet list to make it visually clearer.
src/why_core.nim (2)
185-264: Consider adding timeouts for external command execution.The package manager check functions are well-structured and use
quoteShellfor security. However, theexecCmdcalls have no timeout mechanism, which could cause the tool to hang if a package manager command becomes unresponsive.Consider adding a timeout mechanism to
ctx.execCmdor implementing timeout wrappers for these package manager checks to improve reliability and user experience.
196-206: Consider checking fordnfin the rpm-based system detection.The current implementation distinguishes between "zypper/rpm" and "yum/rpm" based solely on zypper's presence. Modern Fedora systems use
dnf(not yum), so you might want to also check fordnfto provide "dnf/rpm" output for better accuracy.Optional enhancement to detect dnf
proc checkPkgManagerRpm(path: string, ctx: WhyCtx): string = let hasRpm = ctx.findExe("rpm").len > 0 let hasZypper = ctx.findExe("zypper").len > 0 + let hasDnf = ctx.findExe("dnf").len > 0 if not hasRpm: return "" let (outp, exitCode) = ctx.execCmd("rpm -qf " & quoteShell(path)) if exitCode != 0: return "" if hasZypper: return "zypper/rpm (" & outp.strip() & ")" + if hasDnf: + return "dnf/rpm (" & outp.strip() & ")" return "yum/rpm (" & outp.strip() & ")"tests/e2e/gentoo/Dockerfile (1)
9-9: Pin the Gentoo base image to a specific date or digest for reproducibility.Using
gentoo/stage3:latestmakes the build non-deterministic. The Gentoo stage3 images change over time, which could cause tests to break or behave inconsistently without any code changes.Consider using a dated tag or image digest for reproducibility:
-FROM gentoo/stage3:latest +FROM gentoo/stage3:20241220Or use a digest:
-FROM gentoo/stage3:latest +FROM gentoo/stage3@sha256:...
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/why_core.nim(3 hunks)tests/e2e/README.md(1 hunks)tests/e2e/alpine/Dockerfile(1 hunks)tests/e2e/alpine/test.sh(1 hunks)tests/e2e/arch/Dockerfile(1 hunks)tests/e2e/arch/test.sh(1 hunks)tests/e2e/common.sh(1 hunks)tests/e2e/fedora/Dockerfile(1 hunks)tests/e2e/fedora/test.sh(1 hunks)tests/e2e/gentoo/Dockerfile(1 hunks)tests/e2e/gentoo/test.sh(1 hunks)tests/e2e/opensuse/Dockerfile(1 hunks)tests/e2e/opensuse/test.sh(1 hunks)tests/e2e/run.sh(1 hunks)tests/e2e/ubuntu/Dockerfile(1 hunks)tests/e2e/ubuntu/test.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/e2e/opensuse/test.sh
- tests/e2e/fedora/Dockerfile
- tests/e2e/arch/test.sh
- tests/e2e/run.sh
- tests/e2e/arch/Dockerfile
- tests/e2e/alpine/test.sh
- tests/e2e/fedora/test.sh
- tests/e2e/alpine/Dockerfile
🧰 Additional context used
🧬 Code graph analysis (2)
tests/e2e/ubuntu/test.sh (1)
tests/e2e/common.sh (1)
assert_contains(4-13)
tests/e2e/gentoo/test.sh (1)
tests/e2e/common.sh (1)
assert_contains(4-13)
🔇 Additional comments (9)
tests/e2e/README.md (1)
1-30: Documentation structure and content look good.This README clearly explains the purpose of E2E tests, how to run them (with examples), distro coverage, and important operational notes. The references to Alpine's glibc compat layer and Gentoo's stage3 approach are helpful for maintainers.
tests/e2e/gentoo/test.sh (1)
1-10: LGTM! Clean and focused Gentoo e2e test.The test script follows the established e2e testing pattern with strict shell options, proper sourcing of common utilities, and clear provider assertion.
tests/e2e/ubuntu/Dockerfile (1)
1-16: LGTM! Well-structured multi-stage Dockerfile.The two-stage build properly separates compilation from runtime, uses specific version tags, cleans up package lists, and follows Docker best practices.
tests/e2e/opensuse/Dockerfile (1)
1-15: LGTM! Consistent with the established Dockerfile pattern.The multi-stage build follows the same structure as other distro Dockerfiles, uses a specific OpenSUSE version, and properly cleans up after package installation.
src/why_core.nim (2)
53-107: Excellent expansion of provider detection rules.The new provider rules cover a comprehensive set of package and version managers with appropriate path patterns. The dual Windows path separators (both
/and\) ensure cross-platform compatibility.
266-280: Excellent refactoring to a pluggable package manager detection system.The modular design with
PkgManagerRefmakes the code more maintainable and extensible. The first-match strategy is appropriate for the use case.tests/e2e/common.sh (1)
1-13: LGTM! Clean and focused test helper.The
assert_containsfunction usesgrep -qFfor literal matching and provides clear diagnostic output on failure. The strict shell options ensure robust execution.tests/e2e/ubuntu/test.sh (2)
7-23: Well-designed test helpers for comprehensive provider testing.The
make_binandrun_and_assert_providerhelpers provide clean abstractions for creating mock executables and validating provider detection. This makes the test cases below very readable.
29-82: Excellent comprehensive coverage of provider detection.The test suite covers 13 different version managers and package managers with realistic path structures. The rustup test (lines 65-69) is particularly well-designed, correctly simulating rustup's symlink structure from
.cargo/binto the actual toolchain directory.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/e2e/Builder.Dockerfile (2)
3-5: Consider adding--no-install-recommendsflag.Adding the
--no-install-recommendsflag to the apt-get install command would reduce the image size by avoiding unnecessary recommended packages.🔎 Proposed optimization
RUN apt-get update \ - && apt-get install -y --no-install-recommends git ca-certificates \ + && apt-get install -y --no-install-recommends git ca-certificates \ && rm -rf /var/lib/apt/lists/*Wait, I see it already has
--no-install-recommendson line 4. Let me re-check the original code.
6-6: Consider using.dockerignoreto optimize context size.The
COPY . .command copies the entire project directory, which may include unnecessary files like.git, test artifacts, or local build outputs. Using a.dockerignorefile would reduce build context size and improve build performance.tests/e2e/run.sh (1)
16-31: Consider adding binary validation after extraction.The binary extraction logic is correct and includes proper cleanup. However, adding a quick validation step would catch issues early if the wrong architecture is built or the binary is corrupted.
Optional: Add basic validation
docker cp "$container_id":/usr/local/bin/why "$BIN_PATH" docker rm -v "$container_id" >/dev/null chmod +x "$BIN_PATH" + if ! file "$BIN_PATH" | grep -q "ELF 64-bit"; then + echo "Error: Binary is not a valid Linux 64-bit executable" >&2 + return 1 + fi echo "==> Binary ready: ${BIN_PATH}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/ci.yml(1 hunks).gitignore(2 hunks)src/why_core.nim(3 hunks)tests/e2e/Builder.Dockerfile(1 hunks)tests/e2e/README.md(1 hunks)tests/e2e/alpine/Dockerfile(1 hunks)tests/e2e/arch/Dockerfile(1 hunks)tests/e2e/fedora/Dockerfile(1 hunks)tests/e2e/gentoo/Dockerfile(1 hunks)tests/e2e/opensuse/Dockerfile(1 hunks)tests/e2e/run.sh(1 hunks)tests/e2e/ubuntu/Dockerfile(1 hunks)tests/test_why_core.nim(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/alpine/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/gentoo/Dockerfile
- tests/e2e/README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (macos-latest)
- GitHub Check: e2e
🔇 Additional comments (23)
.gitignore (1)
6-6: LGTM!The ignore patterns are appropriate: the e2e binary artifact aligns with the Builder.Dockerfile output, and the
.serena/directory pattern follows standard conventions.Also applies to: 15-18
tests/e2e/Builder.Dockerfile (1)
7-8: LGTM!The build and installation commands are correct. Using
-d:releasefor optimization and setting appropriate executable permissions (0755) are good practices.tests/test_why_core.nim (6)
79-103: LGTM!The zypper detection test is well-structured with appropriate mocking and assertions. It correctly validates the new zypper/rpm package manager detection strategy.
123-139: LGTM!The parameterized test effectively validates path-based detection for all the newly added version managers. The test cases use realistic paths and cover the full scope of changes.
141-154: LGTM!The package manager path detection test provides excellent coverage, including Windows-specific paths with backslashes and spaces. This validates the path normalization logic introduced in the core implementation.
156-178: LGTM!The APK package manager detection test correctly validates Alpine Linux support with appropriate mocking and expected output format.
180-202: LGTM!The pacman test accurately simulates Arch Linux package manager behavior with realistic output format and proper parsing expectations.
204-226: LGTM!The Portage qfile test correctly validates Gentoo package manager support with appropriate output format and parsing logic.
src/why_core.nim (8)
53-58: LGTM!The MacPorts and Nix detection rules use appropriate patterns and match types. The paths align with standard installation locations for these package managers.
72-107: LGTM!Excellent comprehensive coverage of version and package managers. The patterns are accurate and include common installation paths and variations (e.g., fnm's multiple locations, Conda's various distributions, rvm's user/system paths).
161-181: LGTM!The path normalization approach elegantly handles cross-platform path separators without duplicating rules. Normalizing all paths and patterns to forward slashes ensures consistent matching across Windows and Unix systems.
183-187: LGTM!The
PkgManagerStrategytype provides a clean, extensible abstraction for package manager detection. This design makes it easy to add new package managers in the future.
188-197: LGTM!The dpkg detection implementation is secure (uses
quoteShell) and robust (checks for command availability, exit code, and output format). The error handling is appropriate.
199-209: LGTM!The RPM-based detection intelligently distinguishes between zypper (openSUSE) and yum (RHEL/Fedora/CentOS) frontends while using the underlying rpm database. Security and error handling are appropriate.
211-267: LGTM!All four package manager detection functions (apk, pacman, Portage qfile, Portage equery) follow consistent patterns with proper security (quoteShell), error handling, and output parsing. Having both qfile and equery for Portage provides fallback coverage since qfile comes from portage-utils which might not always be installed.
269-283: LGTM!The refactored
checkSystemPackageManagerfunction effectively implements the pluggable strategy pattern. The function iterates through strategies and returns the first successful detection, making it straightforward to add new package managers.tests/e2e/fedora/Dockerfile (1)
1-7: LGTM!The Fedora e2e Dockerfile follows the same clean pattern as other distro test images: minimal package installation, cache cleanup, and straightforward test execution setup.
.github/workflows/ci.yml (1)
46-55: LGTM!The e2e job is well-configured with an appropriate timeout and parallel execution. Running alongside the existing test job provides comprehensive CI coverage.
tests/e2e/ubuntu/Dockerfile (1)
1-8: LGTM!The Ubuntu e2e Dockerfile follows Docker best practices: uses LTS version, minimizes installed packages with
--no-install-recommends, and properly cleans the apt cache to reduce image size.tests/e2e/arch/Dockerfile (1)
1-7: LGTM!The Arch Linux e2e Dockerfile correctly uses pacman commands with appropriate flags for package installation and cache cleanup. The pattern is consistent with other distribution test images.
tests/e2e/opensuse/Dockerfile (1)
1-7: LGTM!The Dockerfile follows best practices: installs necessary packages, cleans up caches, copies test artifacts, and sets a clear entrypoint. The structure is consistent with the e2e test harness pattern.
tests/e2e/run.sh (2)
1-14: LGTM!The script setup follows best practices: strict error handling with
set -euo pipefail, robust root directory discovery, configurable distro list, and sensible parallelism defaults.
33-48: LGTM!The
run_onefunction is well-structured with clear validation, informative progress messages, and appropriate error handling.
1 participant
Summary
Issues
Close #8
Close #9
Close #10
Close #12
Close #13
Close #14
Close #15
Close #16
Close #17
Close #18
Close #19
Close #20
Close #21
Close #22
Close #23
Close #24
Close #25
Close #26
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.