✨ Implement dump-rules subcommand #524
Conversation
Signed-off-by: Alejandro Brugarolas <[email protected]>
Signed-off-by: Alejandro Brugarolas <[email protected]>
WalkthroughA new Changes
Sequence Diagram(s)Dump Rules Command ExecutionsequenceDiagram
participant User
participant CLI
participant DumpRulesCommand
participant Util
participant OS/FileSystem
User->>CLI: Runs "dump-rules --output <dir> [--overwrite]"
CLI->>DumpRulesCommand: Execute RunE
DumpRulesCommand->>Util: GetKantraDir()
Util->>OS/FileSystem: Check current directory and config dirs
Util-->>DumpRulesCommand: Return kantraDir
DumpRulesCommand->>OS/FileSystem: Check rulesets directory
alt Rulesets directory missing
DumpRulesCommand-->>CLI: Log error and exit
else Rulesets directory exists
DumpRulesCommand->>OS/FileSystem: Prepare output zip file
DumpRulesCommand->>OS/FileSystem: Walk rulesets directory, add files to zip
DumpRulesCommand-->>CLI: Log completion
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🧹 Nitpick comments (4)
pkg/util/util.go (1)
255-294: Edge-cases in GetKantraDir(): missing error handling & unclear variable names
The loop treats every
Staterror as “path missing” and silently falls back. For permission-denied (EACCES) or other IO errors this masks real problems.if err != nil && !errors.Is(err, os.ErrNotExist) { return "", err }inside the loop would avoid that.
Using the recycled
setflag after theLookupEnvcall is confusing – it now means “env-var-present”, not “all-reqs-found”. Consider distinct names (allFound,envSet) for readability.
runtime.GOOSis stored inops; plainosType := runtime.GOOSis clearer.cmd/root.go (1)
59-61: Dump-rules command is not assigned to any groupAll other asset-related commands are grouped (
assetGeneration). Ifdump-rulesis meant for end-users rather than internal dev tooling, consider adding it to an appropriatecobra.Groupto keep--helpoutput tidy.cmd/dump-rules.go (1)
122-141: handleOutputFile(): RemoveAll can delete directories unintentionally
os.RemoveAll(output)will silently delete a directory ifoutputis mis-specified.
Useos.Removeand ensurestat.IsDir()is false before deleting.if stat != nil && !stat.IsDir() { return os.Remove(output) }cmd/dump-rules_test.go (1)
1-298: Tests rely on mutable package globals – add t.Parallel() or decoupleSeveral tests modify the global
output/overwritevars. Runninggo test -parallelmay cause data races.
Either:
- call
t.Parallel()inside each sub-test after the globals are snap-shotted & restored with a mutex, or- refactor command code to avoid globals (see earlier comment) and drop these work-arounds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/analyze-bin.go(0 hunks)cmd/analyze.go(1 hunks)cmd/dump-rules.go(1 hunks)cmd/dump-rules_test.go(1 hunks)cmd/root.go(1 hunks)pkg/util/util.go(2 hunks)
💤 Files with no reviewable changes (1)
- cmd/analyze-bin.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
cmd/root.go (1)
cmd/dump-rules.go (1)
NewDumpRulesCommand(22-120)
cmd/analyze.go (1)
pkg/util/util.go (1)
GetKantraDir(255-294)
cmd/dump-rules.go (1)
pkg/util/util.go (2)
GetKantraDir(255-294)RulesetsLocation(23-23)
Signed-off-by: Alejandro Brugarolas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/util/util.go (4)
224-237: Function naming + small robustness improvement
- The name suggests XML-specific logic, but the function is generic. Consider renaming to
IsDirEmptyfor clarity.- Minor optimization: use
Readdirnames(1)and validate the path is a directory before attempting to read.Apply within this range:
-func IsXMLDirEmpty(dir string) (bool, error) { - f, err := os.Open(dir) - if err != nil { - return false, err - } - defer f.Close() - - _, err = f.Readdir(1) - if err == io.EOF { - return true, nil - } - - return false, err -} +func IsXMLDirEmpty(dir string) (bool, error) { + fi, err := os.Stat(dir) + if err != nil { + return false, err + } + if !fi.IsDir() { + return false, fmt.Errorf("%q is not a directory", dir) + } + f, err := os.Open(dir) + if err != nil { + return false, err + } + defer f.Close() + _, err = f.Readdirnames(1) + if err == io.EOF { + return true, nil + } + return false, err +}Optionally add a clearer, non-breaking alias:
// New preferred name. func IsDirEmpty(dir string) (bool, error) { return IsXMLDirEmpty(dir) }
265-277: XDG/OS-specific fallback: prefer XDG conventions, preserve legacy pathFor Linux, it’s conventional to use:
- $XDG_CONFIG_HOME/kantra when set
- $HOME/.config/kantra when not set
Today this returns $HOME/.kantra, which may be intentional for backward compatibility. If you want to adopt XDG but avoid breaking existing users, prefer the XDG path but fall back to legacy $HOME/.kantra if it already exists.
-// fall back to $HOME/.kantra -ops := runtime.GOOS -if ops == "linux" { - dir, set = os.LookupEnv("XDG_CONFIG_HOME") -} -if ops != "linux" || dir == "" || !set { - // on Unix, including macOS, this returns the $HOME environment variable. On Windows, it returns %USERPROFILE% - dir, err = os.UserHomeDir() - if err != nil { - return "", err - } -} -return filepath.Join(dir, ".kantra"), nil +osName := runtime.GOOS +if osName == "linux" { + if xdg, ok := os.LookupEnv("XDG_CONFIG_HOME"); ok && xdg != "" { + return filepath.Join(xdg, "kantra"), nil + } + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + // Back-compat: if legacy exists, keep using it; otherwise follow XDG. + legacy := filepath.Join(home, ".kantra") + if _, err := os.Stat(legacy); err == nil { + return legacy, nil + } + return filepath.Join(home, ".config", "kantra"), nil +} +// Non-Linux: keep existing behavior. +home, err := os.UserHomeDir() +if err != nil { + return "", err +} +return filepath.Join(home, ".kantra"), nil
239-278: Add env override (KANTRA_DIR) to aid testing and power usersConsider allowing an explicit override via env (e.g., KANTRA_DIR). This helps CI/tests and lets users colocate content as needed.
Example:
if v := os.Getenv("KANTRA_DIR"); v != "" { return v, nil }If you want, I can patch this in and add tests.
243-263: Verify directory-check assumption in pkg/util/util.go (lines 243–263)The current loop only checks for existence (which may be regular files). To prevent false positives, ensure each path is a directory:
- for _, v := range reqs { - _, err := os.Stat(filepath.Join(dir, v)) - if err != nil { - set = false - break - } - } + for _, v := range reqs { + p := filepath.Join(dir, v) + info, err := os.Stat(p) + if err != nil || !info.IsDir() { + set = false + break + } + }However, the quick check in the repo root showed none of these directories exist locally:
- Missing dir: rulesets
- Missing dir: jdtls
- Missing dir: static-report
Please confirm that in your packaged/installed environment (or wherever this runs), all three of these are guaranteed to be present as directories in the working directory. If so, apply the above diff to harden the check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/analyze-bin.go(0 hunks)cmd/analyze.go(1 hunks)cmd/dump-rules.go(1 hunks)pkg/util/util.go(2 hunks)
💤 Files with no reviewable changes (1)
- cmd/analyze-bin.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/dump-rules.go
- cmd/analyze.go
⏰ 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). (1)
- GitHub Check: Build & test from commit
🔇 Additional comments (2)
pkg/util/util.go (2)
11-11: Import looks correct and necessary
runtimeis used byGetKantraDir()to branch on OS. No issues.
239-278: All call sites updated to use util.GetKantraDir()Verified that there are no remaining calls to
setKantraDir()and that bothcmd/dump-rules.goandcmd/analyze.gonow correctly invokeutil.GetKantraDir().
No further changes required.
Signed-off-by: Alejandro Brugarolas <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
==========================================
- Coverage 21.73% 21.68% -0.06%
==========================================
Files 32 33 +1
Lines 4642 4755 +113
==========================================
+ Hits 1009 1031 +22
- Misses 3516 3603 +87
- Partials 117 121 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Alejandro Brugarolas <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
cmd/dump-rules_test.go (7)
117-124: Avoid brittle equality checks for command textsExact string equality on
UseandShortis brittle. Prefer substring or prefix checks to reduce churn from minor text edits.- if cmd.Use != "dump-rules" { - t.Errorf("expected command Use to be 'dump-rules', got '%s'", cmd.Use) - } + if !strings.Contains(cmd.Use, "dump-rules") { + t.Errorf("expected command Use to contain 'dump-rules', got '%s'", cmd.Use) + } - if cmd.Short != "Dump builtin rulesets" { - t.Errorf("expected command Short to be 'Dump builtin rulesets', got '%s'", cmd.Short) - } + if !strings.Contains(strings.ToLower(cmd.Short), "dump") || + !strings.Contains(strings.ToLower(cmd.Short), "ruleset") { + t.Errorf("unexpected Short: %q", cmd.Short) + }
151-162: Reduce flakiness when checking flag usage stringsUsage texts evolve. Assert on meaningful substrings rather than full equality.
- if outputFlag.Usage != "path to the directory for rulesets output" { - t.Errorf("unexpected output flag usage: %s", outputFlag.Usage) - } + if !strings.Contains(outputFlag.Usage, "directory") || + !strings.Contains(strings.ToLower(outputFlag.Usage), "ruleset") { + t.Errorf("unexpected output flag usage: %s", outputFlag.Usage) + } - if overwriteFlag.Usage != "overwrite output directory" { - t.Errorf("unexpected overwrite flag usage: %s", overwriteFlag.Usage) - } + if !strings.Contains(strings.ToLower(overwriteFlag.Usage), "overwrite") { + t.Errorf("unexpected overwrite flag usage: %s", overwriteFlag.Usage) + }
106-109: Silence test logs to keep CI output cleanRoute logs to io.Discard to avoid noisy test output.
- logrusLog := logrus.New() - logrusLog.SetOutput(os.Stdout) + logrusLog := logrus.New() + logrusLog.SetOutput(io.Discard) logger := logrusr.New(logrusLog)Also applies to: 134-137
164-172: Consolidate duplicate “missing output flag” tests and relax exact error textBoth sections validate the “required output flag” error. Keep one assertion path and avoid strict matches to Cobra’s exact phrasing, which varies by version.
- Remove the “should fail when no output flag provided” case in one of the tests, or keep it only in TestDumpRulesCommandFlagValidation.
- In the remaining test, check that an error occurs and that it contains a stable substring like “required flag”:
- errorMsg: "required flag(s) \"output\" not set", + errorMsg: "required flag",This reduces brittleness across Cobra versions.
Also applies to: 174-200, 201-220
59-63: Also assert preservation when overwrite=false and file already existsCurrently, we check deletion on overwrite=true, but we don’t assert that an existing file remains when overwrite=false. Capture pre-state and verify.
- // Setup test file if needed - if err := tt.setupFunc(testFile); err != nil { - t.Fatal(err) - } + // Setup test file if needed + if err := tt.setupFunc(testFile); err != nil { + t.Fatal(err) + } + // Track whether the file existed before calling handleOutputFile + _, statErr := os.Stat(testFile) + existedBefore := !os.IsNotExist(statErr) ... - // Verify overwrite behavior - if tt.overwriteFlag && tt.expectError == false && tt.setupFunc != nil { + // Verify overwrite/preservation behavior + if tt.overwriteFlag && !tt.expectError && tt.setupFunc != nil { // File should be removed when overwrite is true if _, err := os.Stat(testFile); !os.IsNotExist(err) { t.Error("file should be removed when overwrite is true") } + } else if !tt.overwriteFlag && existedBefore { + // File should still exist when overwrite is false + if _, err := os.Stat(testFile); os.IsNotExist(err) { + t.Error("file should not be removed when overwrite is false") + } }Also applies to: 92-99
30-37: Loosen the expected error substring for existing file without overwriteThe current expected substring is highly specific and ties to exact phrasing. Prefer stable keywords.
- errorSubstring: "already exists and --overwrite not set", + errorSubstring: "already exists",
103-129: Consider adding an end-to-end smoke test for archive creationAdd a test that:
- Creates a temp output dir.
- Executes the command with
--output <dir>(and--overwrite).- Verifies the archive file exists and is non-empty.
Also consider a case where the parent directory does not exist to confirm it’s created as needed.
I can scaffold this test and, if needed, help introduce a seam (e.g., injectable ruleset source) to avoid depending on the real Kantra directory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/dump-rules.go(1 hunks)cmd/dump-rules_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/dump-rules.go
⏰ 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). (1)
- GitHub Check: Build & test from commit
| // Unit test for output path handling logic | ||
| func TestOutputPathHandling(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| outputDir string | ||
| expectFilename string | ||
| }{ | ||
| { | ||
| name: "should append default filename to directory", | ||
| outputDir: "/tmp/output", | ||
| expectFilename: "default-rulesets.zip", | ||
| }, | ||
| { | ||
| name: "should handle relative paths", | ||
| outputDir: "./output", | ||
| expectFilename: "default-rulesets.zip", | ||
| }, | ||
| { | ||
| name: "should handle empty path", | ||
| outputDir: "", | ||
| expectFilename: "default-rulesets.zip", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // This simulates the logic from line 32 in dump-rules.go | ||
| result := filepath.Join(tt.outputDir, "default-rulesets.zip") | ||
| expectedPath := filepath.Join(tt.outputDir, tt.expectFilename) | ||
|
|
||
| if result != expectedPath { | ||
| t.Errorf("expected path '%s', got '%s'", expectedPath, result) | ||
| } | ||
|
|
||
| // Verify the filename is always present | ||
| filename := filepath.Base(result) | ||
| if filename != tt.expectFilename { | ||
| t.Errorf("expected filename '%s', got '%s'", tt.expectFilename, filename) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate archive format (tarball vs zip) and de-brittle default filename in tests
Issue #229 asks for a tarball, but these tests assume a zip (default-rulesets.zip). This can cause a divergence between spec and implementation and make tests fail if the code uses .tar.gz.
- Verify what the implementation actually creates (zip vs tar.gz) and align tests accordingly.
- Centralize the default archive filename in a single constant to simplify changes.
Proposed changes within this test (parameterize the filename and use it consistently):
- result := filepath.Join(tt.outputDir, "default-rulesets.zip")
- expectedPath := filepath.Join(tt.outputDir, tt.expectFilename)
+ result := filepath.Join(tt.outputDir, defaultArchiveName)
+ expectedPath := filepath.Join(tt.outputDir, tt.expectFilename)And update the table values accordingly:
- expectFilename: "default-rulesets.zip",
+ expectFilename: defaultArchiveName,Additionally, add this helper near the top of the file (outside this hunk) so tests don’t hardcode the name everywhere:
// Keep this in sync with the implementation (zip vs tar.gz)
const defaultArchiveName = "default-rulesets.zip"If the implementation uses tarballs per the issue, switch the constant to:
const defaultArchiveName = "default-rulesets.tar.gz"To help confirm the implementation details, run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate dump-rules command implementation"
rg -n "NewDumpRulesCommand|dump-rules" -g "cmd/**"
echo
echo "Show flags and descriptions for output/overwrite"
rg -n 'Flags\(\)|PersistentFlags\(\)' -A5 -B2 -g "cmd/**"
rg -n '"output"|--output|\"overwrite\"|--overwrite' -A2 -B2 -g "cmd/**"
echo
echo "Search for default archive name and format usage"
rg -n 'default-rulesets|\.zip|\.tar\.gz|archive/zip|archive/tar|compress/gzip' -g "**/*.go" -A2 -B2Length of output: 17526
---
🏁 Script executed:
```shell
#!/bin/bash
set -euo pipefail
# Show the code for NewDumpRulesCommand and how output filename is computed
echo "==== NewDumpRulesCommand (cmd/dump-rules.go) ===="
sed -n '1,200p' cmd/dump-rules.go
echo
echo "==== Look for handleOutputFile or similar helpers ===="
grep -R "handleOutput" -n cmd/dump-rules.go || echo "No handleOutputFile found"
echo
echo "==== Inspect RunE block for default filename logic ===="
awk '/RunE:/,/^ *},/p' cmd/dump-rules.go
Length of output: 3830
Centralize the default archive name in tests to reduce duplication and prevent future mismatches
The implementation in cmd/dump-rules.go always appends “default-rulesets.zip” (zip format) as the output filename, and the tests in cmd/dump-rules_test.go hardcode this string in multiple places. While the format is correct today, centralizing it behind a single constant will:
- Make updates easier if the archive format ever changes
- Remove duplicated literals from test cases
- Simplify the TestOutputPathHandling table (the filename is the same across cases)
Suggested changes in cmd/dump-rules_test.go:
-
At the top of the file, add:
// defaultArchiveName must match the implementation in cmd/dump-rules.go const defaultArchiveName = "default-rulesets.zip"
-
Simplify TestOutputPathHandling by removing the expectFilename field and using the constant:
func TestOutputPathHandling(t *testing.T) { - tests := []struct { - name string - outputDir string - expectFilename string - }{ - { name: "append default filename", outputDir: "/tmp/output", expectFilename: "default-rulesets.zip" }, - { name: "relative path", outputDir: "./output", expectFilename: "default-rulesets.zip" }, - { name: "empty path", outputDir: "", expectFilename: "default-rulesets.zip" }, - } + tests := []struct { + name string + outputDir string + }{ + { name: "append default filename", outputDir: "/tmp/output" }, + { name: "relative path", outputDir: "./output" }, + { name: "empty path", outputDir: "" }, + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := filepath.Join(tt.outputDir, "default-rulesets.zip") - expectedPath := filepath.Join(tt.outputDir, tt.expectFilename) + result := filepath.Join(tt.outputDir, defaultArchiveName) + expectedPath := filepath.Join(tt.outputDir, defaultArchiveName) if result != expectedPath { t.Errorf("expected path '%s', got '%s'", expectedPath, result) } filename := filepath.Base(result) - if filename != tt.expectFilename { + if filename != defaultArchiveName { t.Errorf("expected filename '%s', got '%s'", defaultArchiveName, filename) } }) } }
These changes keep the test aligned with the implementation today and make it trivial to switch to a .tar.gz filename in one place if the spec ever changes.
🤖 Prompt for AI Agents
In cmd/dump-rules_test.go around lines 222 to 263, the test hardcodes the
default archive filename "default-rulesets.zip" multiple times, causing
duplication and risk of mismatch with the implementation. To fix this, define a
constant defaultArchiveName at the top of the test file with the value
"default-rulesets.zip" matching the implementation, then remove the
expectFilename field from the test cases and replace all occurrences of the
hardcoded filename in TestOutputPathHandling with this constant. This
centralizes the filename, reduces duplication, and simplifies future updates.
There was a problem hiding this comment.
@jmle Thanks for the positive feedback! I'm glad the suggestion to centralize the default archive filename resonates with you. It's always satisfying when we can identify opportunities to make the codebase more maintainable and reduce the risk of inconsistencies between implementation and tests.
If you'd like me to help with any other aspects of this PR or have additional questions, feel free to let me know!
🧠 Learnings used
Learnt from: abrugaro
PR: konveyor/kantra#524
File: cmd/dump-rules.go:0-0
Timestamp: 2025-08-08T15:06:52.056Z
Learning: In konveyor/kantra cmd/dump-rules.go (Go/Cobra), the dump-rules command should not use package-level variables for flags (output, overwrite). Flags must be defined as local variables inside NewDumpRulesCommand to avoid shared mutable state across commands/tests.
| if set { | ||
| return dir, nil | ||
| } | ||
| // fall back to $HOME/.kantra |
There was a problem hiding this comment.
I wonder if we should also add the ability to set a $KANTRA_HOME directory, but we can leave that for a later discussion.
jmle
left a comment
jmle
left a comment
There was a problem hiding this comment.
Looks great! Let's just address the nit from code rabbit and the code coverage complaint in the built 👍
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. |
|
This PR is still relevant and implements a useful feature from #229! ✅ However, there are a few issues to address: 1. Code Coverage Issue ❌The codecov/patch check is failing with only 14.67% coverage (128 lines missing). The 2. Overlapping Bug with PR #465
|
3 participants
Resolves #229
Summary by CodeRabbit