Add test to verify dep output#62
Open
eemcmullan wants to merge 2 commits into
Open
Conversation
Signed-off-by: Emily McMullan <emcmulla@redhat.com>
📝 WalkthroughWalkthroughThe changes introduce a comprehensive dependency handling system for the test framework, adding configuration fields, parsing/normalization utilities, validation logic, and enhanced Hub integration to support dependency manifest generation and validation for both Kantra and Hub targets. Changes
Sequence Diagram(s)sequenceDiagram
participant Test Framework
participant Config Loader
participant Kantra/Hub Target
participant Parser
participant Validator
participant CLI Output
Test Framework->>Config Loader: Load test definition + expected deps
Config Loader->>Config Loader: LoadExpectedDependencies() for kantra/hub
Config Loader-->>Test Framework: Return test config with DepItems
alt Generate Command
Test Framework->>Kantra/Hub Target: Execute analysis
Kantra/Hub Target->>Kantra/Hub Target: Emit dependencies.yaml
Test Framework->>Parser: ParseDependencies(dependencies.yaml)
Parser-->>Test Framework: Parsed DepsFlatItem list
Test Framework->>Parser: NormalizeDepsFlat(parsed, testDir)
Parser-->>Test Framework: Normalized & sorted items
Test Framework->>CLI Output: Save expected-deps-*.yaml
end
alt Run Test Command
Test Framework->>Kantra/Hub Target: Execute analysis
Kantra/Hub Target->>Kantra/Hub Target: Emit dependencies.yaml
Test Framework->>Parser: ParseDependencies(dependencies.yaml)
Parser-->>Test Framework: Parsed actual deps
Test Framework->>Parser: NormalizeDepsFlat(actual, testDir)
Parser-->>Test Framework: Normalized actual deps
Test Framework->>Validator: ValidateDependenciesFlat(actual, expected)
Validator-->>Test Framework: ValidationResult with errors
Test Framework->>Test Framework: Update test result (pass/fail)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 2
🧹 Nitpick comments (2)
pkg/parser/deps.go (2)
74-82: Considerstrings.TrimPrefixinstead ofReplaceAll.
strings.ReplaceAllreplaces all occurrences oftestDirin the path. IftestDirappears elsewhere in the path (e.g., in a directory name), it could cause unintended modifications.Suggested fix using TrimPrefix
func normalizeDepsPathLike(s, testDir string) string { if s == "" || testDir == "" { return NormalizePath(s) } normalizedTestDir := filepath.ToSlash(testDir) s = filepath.ToSlash(s) - s = strings.ReplaceAll(s, normalizedTestDir, "") + s = strings.TrimPrefix(s, normalizedTestDir) return NormalizePath(s) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/deps.go` around lines 74 - 82, The function normalizeDepsPathLike currently uses strings.ReplaceAll to strip normalizedTestDir from s which can remove occurrences beyond the path prefix; update normalizeDepsPathLike to use strings.TrimPrefix on the filepath.ToSlash(s) with normalizedTestDir so only a leading testDir is removed, then pass the result to NormalizePath; keep the existing empty-string guard and use filepath.ToSlash for both inputs as before.
31-60: Unused error return value.The function signature returns an error, but the function always returns
nil. Either propagate errors from internal operations (e.g., line 70 ignores error fromnormalizeIncident), or simplify the signature to return only[]konveyor.DepsFlatItem.Option 1: Simplify signature if errors aren't expected
-func NormalizeDepsFlat(items []konveyor.DepsFlatItem, testDir string) ([]konveyor.DepsFlatItem, error) { +func NormalizeDepsFlat(items []konveyor.DepsFlatItem, testDir string) []konveyor.DepsFlatItem {(Would require updating callers accordingly)
Option 2: Propagate errors from normalizeIncident
func normalizeDepsFileURI(s, testDir string) string { +func normalizeDepsFileURI(s, testDir string) (string, error) { if s == "" { - return s + return s, nil } if !strings.Contains(s, "file://") { - return NormalizePath(normalizeDepsPathLike(s, testDir)) + return NormalizePath(normalizeDepsPathLike(s, testDir)), nil } inc := konveyor.Incident{URI: uri.URI(s)} - out, _ := normalizeIncident(inc, testDir) - return string(out.URI) + out, err := normalizeIncident(inc, testDir) + if err != nil { + return "", err + } + return string(out.URI), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/deps.go` around lines 31 - 60, The NormalizeDepsFlat function currently returns ([]konveyor.DepsFlatItem, error) but never returns a non-nil error; either remove the error from the signature or actually propagate errors from the internal normalization helpers. Pick one: (A) simplify the signature of NormalizeDepsFlat to return only []konveyor.DepsFlatItem and update all callers, or (B) change calls to normalizeDepsFileURI, normalizeDepExtrasMap and any normalizeIncident usage inside NormalizeDepsFlat to capture and check returned errors and return them (e.g., if normalizeDepsFileURI or normalizeDepExtrasMap can fail, handle the error and return nil, err). Update the signature and callers consistently so the unused error return is eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/generate.go`:
- Around line 233-248: The code currently only sets test.Expect.Output.DepFile
when dependencies.yaml exists and leaves any prior value in place when it does
not, and it silently ignores os.Stat errors other than "not found"; update the
block around depsOutPath and os.Stat so that: if os.Stat returns nil, parse and
save as before (using parser.ParseDependencies and saveNormalizedDepsFlat) and
set test.Expect.Output.DepFile = "expected-deps-output.yaml"; if os.Stat signals
"not found" (os.IsNotExist), explicitly clear test.Expect.Output.DepFile (set to
empty string) to avoid stale values; if os.Stat returns any other error, treat
it as a failure (log via color.Red or increment failCount and continue) rather
than ignoring it. Ensure references to depsOutPath, parser.ParseDependencies,
saveNormalizedDepsFlat, and test.Expect.Output.DepFile are used to locate and
modify the logic.
In `@tests/book-server-deps/expected-deps-output.yaml`:
- Around line 1-759: The baseline fixture is polluted with environment-specific
absolute URIs (notably the fileURI field "file:///opt/input/source/pom.xml" and
many prefix values like "file:///addon/.m2/repository/..."); update the YAML
fixture (tests/book-server-deps/expected-deps-output.yaml) to use normalized,
non-machine-specific values by removing or replacing absolute paths in the
fileURI and prefix fields (e.g., use relative paths or a stable placeholder such
as "<REPO_ROOT>/pom.xml" and "<M2_REPO>/..."), and ensure the generator that
created this fixture emits those normalized forms for the fileURI and prefix
fields so the baseline no longer churns on environment changes.
---
Nitpick comments:
In `@pkg/parser/deps.go`:
- Around line 74-82: The function normalizeDepsPathLike currently uses
strings.ReplaceAll to strip normalizedTestDir from s which can remove
occurrences beyond the path prefix; update normalizeDepsPathLike to use
strings.TrimPrefix on the filepath.ToSlash(s) with normalizedTestDir so only a
leading testDir is removed, then pass the result to NormalizePath; keep the
existing empty-string guard and use filepath.ToSlash for both inputs as before.
- Around line 31-60: The NormalizeDepsFlat function currently returns
([]konveyor.DepsFlatItem, error) but never returns a non-nil error; either
remove the error from the signature or actually propagate errors from the
internal normalization helpers. Pick one: (A) simplify the signature of
NormalizeDepsFlat to return only []konveyor.DepsFlatItem and update all callers,
or (B) change calls to normalizeDepsFileURI, normalizeDepExtrasMap and any
normalizeIncident usage inside NormalizeDepsFlat to capture and check returned
errors and return them (e.g., if normalizeDepsFileURI or normalizeDepExtrasMap
can fail, handle the error and return nil, err). Update the signature and
callers consistently so the unused error return is eliminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ce36ced-2294-431d-bb14-0566fcb81cfa
📒 Files selected for processing (10)
Makefilepkg/cli/generate.gopkg/cli/run.gopkg/config/loader.gopkg/config/types.gopkg/parser/deps.gopkg/parser/deps_test.gopkg/validator/deps.gotests/book-server-deps/expected-deps-output.yamltests/book-server-deps/test.yaml
Comment on lines
+233
to
+248
| depsOutPath := filepath.Join(filepath.Dir(result.OutputFile), "dependencies.yaml") | ||
| if _, statErr := os.Stat(depsOutPath); statErr == nil { | ||
| depsList, err := parser.ParseDependencies(depsOutPath) | ||
| if err != nil { | ||
| color.Red(" ✗ Failed to parse dependencies.yaml: %v", err) | ||
| failCount++ | ||
| continue | ||
| } | ||
| expectedDepsFile := filepath.Join(testDirPath, "expected-deps-output.yaml") | ||
| if err := saveNormalizedDepsFlat(depsList, expectedDepsFile, testDirPath); err != nil { | ||
| color.Red(" ✗ Failed to save expected dependencies: %v", err) | ||
| failCount++ | ||
| continue | ||
| } | ||
| test.Expect.Output.DepFile = "expected-deps-output.yaml" | ||
| } |
There was a problem hiding this comment.
Clear stale dep-file and handle non-ENOENT Stat failures.
At Line 247, DepFile is only assigned when dependencies.yaml exists, but it is never reset when it does not. Regeneration can therefore preserve a stale dep-file from prior runs and trigger false validation failures later. Also, non-not found stat errors are currently ignored.
🔧 Proposed fix
import (
"context"
+ "errors"
"fmt"
"os"
"path/filepath"
"strings"
@@
- depsOutPath := filepath.Join(filepath.Dir(result.OutputFile), "dependencies.yaml")
- if _, statErr := os.Stat(depsOutPath); statErr == nil {
+ depsOutPath := filepath.Join(filepath.Dir(result.OutputFile), "dependencies.yaml")
+ test.Expect.Output.DepFile = ""
+ if _, statErr := os.Stat(depsOutPath); statErr == nil {
depsList, err := parser.ParseDependencies(depsOutPath)
if err != nil {
color.Red(" ✗ Failed to parse dependencies.yaml: %v", err)
@@
}
test.Expect.Output.DepFile = "expected-deps-output.yaml"
+ } else if !errors.Is(statErr, os.ErrNotExist) {
+ color.Red(" ✗ Failed to stat dependencies.yaml: %v", statErr)
+ failCount++
+ continue
}📝 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.
Suggested change
| depsOutPath := filepath.Join(filepath.Dir(result.OutputFile), "dependencies.yaml") | |
| if _, statErr := os.Stat(depsOutPath); statErr == nil { | |
| depsList, err := parser.ParseDependencies(depsOutPath) | |
| if err != nil { | |
| color.Red(" ✗ Failed to parse dependencies.yaml: %v", err) | |
| failCount++ | |
| continue | |
| } | |
| expectedDepsFile := filepath.Join(testDirPath, "expected-deps-output.yaml") | |
| if err := saveNormalizedDepsFlat(depsList, expectedDepsFile, testDirPath); err != nil { | |
| color.Red(" ✗ Failed to save expected dependencies: %v", err) | |
| failCount++ | |
| continue | |
| } | |
| test.Expect.Output.DepFile = "expected-deps-output.yaml" | |
| } | |
| depsOutPath := filepath.Join(filepath.Dir(result.OutputFile), "dependencies.yaml") | |
| test.Expect.Output.DepFile = "" | |
| if _, statErr := os.Stat(depsOutPath); statErr == nil { | |
| depsList, err := parser.ParseDependencies(depsOutPath) | |
| if err != nil { | |
| color.Red(" ✗ Failed to parse dependencies.yaml: %v", err) | |
| failCount++ | |
| continue | |
| } | |
| expectedDepsFile := filepath.Join(testDirPath, "expected-deps-output.yaml") | |
| if err := saveNormalizedDepsFlat(depsList, expectedDepsFile, testDirPath); err != nil { | |
| color.Red(" ✗ Failed to save expected dependencies: %v", err) | |
| failCount++ | |
| continue | |
| } | |
| test.Expect.Output.DepFile = "expected-deps-output.yaml" | |
| } else if !errors.Is(statErr, os.ErrNotExist) { | |
| color.Red(" ✗ Failed to stat dependencies.yaml: %v", statErr) | |
| failCount++ | |
| continue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cli/generate.go` around lines 233 - 248, The code currently only sets
test.Expect.Output.DepFile when dependencies.yaml exists and leaves any prior
value in place when it does not, and it silently ignores os.Stat errors other
than "not found"; update the block around depsOutPath and os.Stat so that: if
os.Stat returns nil, parse and save as before (using parser.ParseDependencies
and saveNormalizedDepsFlat) and set test.Expect.Output.DepFile =
"expected-deps-output.yaml"; if os.Stat signals "not found" (os.IsNotExist),
explicitly clear test.Expect.Output.DepFile (set to empty string) to avoid stale
values; if os.Stat returns any other error, treat it as a failure (log via
color.Red or increment failCount and continue) rather than ignoring it. Ensure
references to depsOutPath, parser.ParseDependencies, saveNormalizedDepsFlat, and
test.Expect.Output.DepFile are used to locate and modify the logic.
Comment on lines
+1
to
+759
| - name: org.springframework.spring-expression | ||
| version: 5.1.2.RELEASE | ||
| type: compile | ||
| indirect: true | ||
| resolvedIdentifier: 7f45b616d59579e07c44ae01b5fc5821db19b322 | ||
| extras: | ||
| artifactId: spring-expression | ||
| baseDep: | ||
| extras: | ||
| artifactId: spring-boot-starter-web | ||
| groupId: org.springframework.boot | ||
| pomPath: /opt/input/source/pom.xml | ||
| name: org.springframework.boot.spring-boot-starter-web | ||
| version: 2.1.0.RELEASE | ||
| groupId: org.springframework | ||
| pomPath: /opt/input/source/pom.xml | ||
| labels: | ||
| - konveyor.io/dep-source=open-source | ||
| - konveyor.io/language=java | ||
| prefix: file:///addon/.m2/repository/org/springframework/spring-expression/5.1.2.RELEASE | ||
| - name: org.springframework.spring-jcl | ||
| version: 5.1.2.RELEASE | ||
| type: compile | ||
| indirect: true | ||
| resolvedIdentifier: b35f3aeb0afcc782ec7a1c1f1ab8325fb7a9e223 | ||
| extras: | ||
| artifactId: spring-jcl | ||
| baseDep: | ||
| extras: | ||
| artifactId: spring-boot-starter-web | ||
| groupId: org.springframework.boot | ||
| pomPath: /opt/input/source/pom.xml | ||
| name: org.springframework.boot.spring-boot-starter-web | ||
| version: 2.1.0.RELEASE | ||
| groupId: org.springframework | ||
| pomPath: /opt/input/source/pom.xml | ||
| labels: | ||
| - konveyor.io/dep-source=open-source | ||
| - konveyor.io/language=java | ||
| prefix: file:///addon/.m2/repository/org/springframework/spring-jcl/5.1.2.RELEASE | ||
| - name: org.springframework.spring-web | ||
| version: 5.1.2.RELEASE | ||
| type: compile | ||
| indirect: true | ||
| resolvedIdentifier: ed47f5756ab15e223ac5cb4993beb8bfa5adc130 | ||
| extras: | ||
| artifactId: spring-web | ||
| baseDep: | ||
| extras: | ||
| artifactId: spring-boot-starter-web | ||
| groupId: org.springframework.boot | ||
| pomPath: /opt/input/source/pom.xml | ||
| name: org.springframework.boot.spring-boot-starter-web | ||
| version: 2.1.0.RELEASE | ||
| groupId: org.springframework | ||
| pomPath: /opt/input/source/pom.xml | ||
| labels: | ||
| - konveyor.io/dep-source=open-source | ||
| - konveyor.io/language=java | ||
| prefix: file:///addon/.m2/repository/org/springframework/spring-web/5.1.2.RELEASE | ||
| - name: org.springframework.spring-webmvc | ||
| version: 5.1.2.RELEASE | ||
| type: compile | ||
| indirect: true | ||
| resolvedIdentifier: ee1900a33a5376cb0b9514cc29db80aa6bf7c291 | ||
| extras: | ||
| artifactId: spring-webmvc | ||
| baseDep: | ||
| extras: | ||
| artifactId: spring-boot-starter-web | ||
| groupId: org.springframework.boot | ||
| pomPath: /opt/input/source/pom.xml | ||
| name: org.springframework.boot.spring-boot-starter-web | ||
| version: 2.1.0.RELEASE | ||
| groupId: org.springframework | ||
| pomPath: /opt/input/source/pom.xml | ||
| labels: | ||
| - konveyor.io/dep-source=open-source | ||
| - konveyor.io/language=java | ||
| prefix: file:///addon/.m2/repository/org/springframework/spring-webmvc/5.1.2.RELEASE | ||
| - name: org.yaml.snakeyaml | ||
| version: "1.23" | ||
| type: runtime | ||
| indirect: true | ||
| resolvedIdentifier: 0542a3f938aebd15b1ae655c89344835e502a87d | ||
| extras: | ||
| artifactId: snakeyaml | ||
| baseDep: | ||
| extras: | ||
| artifactId: spring-boot-starter-web | ||
| groupId: org.springframework.boot | ||
| pomPath: /opt/input/source/pom.xml | ||
| name: org.springframework.boot.spring-boot-starter-web | ||
| version: 2.1.0.RELEASE | ||
| groupId: org.yaml | ||
| pomPath: /opt/input/source/pom.xml | ||
| labels: | ||
| - konveyor.io/dep-source=open-source | ||
| - konveyor.io/language=java | ||
| prefix: file:///addon/.m2/repository/org/yaml/snakeyaml/1.23 |
There was a problem hiding this comment.
Fixture appears non-canonical for a “normalized deps output” baseline.
Line 1 (file:///opt/input/source/...) and many prefix entries (e.g., Line 23 with file:///addon/.m2/...) are environment-specific. This makes the committed baseline noisier than necessary and likely to churn when regenerated via the new normalized generation flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/book-server-deps/expected-deps-output.yaml` around lines 1 - 759, The
baseline fixture is polluted with environment-specific absolute URIs (notably
the fileURI field "file:///opt/input/source/pom.xml" and many prefix values like
"file:///addon/.m2/repository/..."); update the YAML fixture
(tests/book-server-deps/expected-deps-output.yaml) to use normalized,
non-machine-specific values by removing or replacing absolute paths in the
fileURI and prefix fields (e.g., use relative paths or a stable placeholder such
as "<REPO_ROOT>/pom.xml" and "<M2_REPO>/..."), and ensure the generator that
created this fixture emits those normalized forms for the fileURI and prefix
fields so the baseline no longer churns on environment changes.
Signed-off-by: Emily McMullan <emcmulla@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/targets/tackle_hub_attachments.go (1)
125-135: Deferred cleanup inside loop accumulates until function returns.The
defer os.RemoveAll(sub)at line 129 is inside the loop, so all temp directories accumulate untilfindDepsYAMLInTreereturns. With many tar archives, this could temporarily consume more disk space than necessary.♻️ Optional: extract to helper for immediate cleanup
+func (t *TackleHubTarget) tryExtractDepsFromTar(tarPath string) (string, error) { + sub, err := os.MkdirTemp("", "koncur-hub-tar-*") + if err != nil { + return "", err + } + defer os.RemoveAll(sub) + if err := extractTarArchive(tarPath, sub); err != nil { + return "", fmt.Errorf("extract %s: %w", filepath.Base(tarPath), err) + } + if p, err := walkForDepsYAML(sub); err == nil { + // Copy file out before cleanup + dst, _ := os.CreateTemp("", "deps-*.yaml") + dst.Close() + if err := copyFile(p, dst.Name()); err != nil { + return "", err + } + return dst.Name(), nil + } + return "", os.ErrNotExist +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/targets/tackle_hub_attachments.go` around lines 125 - 135, The defer os.RemoveAll(sub) inside the loop in findDepsYAMLInTree causes all temp dirs created by os.MkdirTemp to remain until the function returns; change the logic so each temporary directory is removed immediately after it's no longer needed: create the temp dir (os.MkdirTemp), call extractTarArchive and then call walkForDepsYAML on it, and if walkForDepsYAML returns a path return it after removing the temp dir, otherwise remove the temp dir immediately (or encapsulate the extract+inspect+cleanup into a helper that ensures os.RemoveAll(sub) runs before continuing); reference extractTarArchive, walkForDepsYAML and os.RemoveAll to locate and implement the change.pkg/config/types.go (1)
143-159: Consider documenting behavior for unsupported target types.The method handles
"tackle-hub"and"kantra"explicitly, with adefaultcase falling back toDepItemsKantra. For other target types ("tackle-ui","kai-rpc","vscode"), if onlyDepFileHubis configured (withoutDepFileKantra), the method returnsnil, false, silently skipping dependency validation.This asymmetry may be intentional (those targets might not support dependency validation), but a brief doc comment would clarify the expected behavior.
📝 Suggested documentation
+// ExpectedDepItemsForTarget returns the expected dependency items for the given target type. +// For "tackle-hub", it returns DepItemsHub; for "kantra", it returns DepItemsKantra. +// For other target types (tackle-ui, kai-rpc, vscode), it falls back to DepItemsKantra +// if configured, otherwise returns nil (no dependency validation). func (e *ExpectedOutput) ExpectedDepItemsForTarget(targetType string) ([]konveyor.DepsFlatItem, bool) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/types.go` around lines 143 - 159, Add a doc comment to the ExpectedDepItemsForTarget method explaining its behavior for unsupported/other target types: state that it explicitly handles "tackle-hub" (returns DepItemsHub when DepFileHub is set) and "kantra" (returns DepItemsKantra when DepFileKantra is set), and that the default case falls back to kantra semantics (i.e., will only return items if DepFileKantra is set), so targets like "tackle-ui", "kai-rpc", "vscode" will not perform dependency validation unless DepFileKantra is present; reference the method ExpectedDepItemsForTarget and the fields DepFileHub, DepItemsHub, DepFileKantra, DepItemsKantra in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/targets/tackle_hub_attachments.go`:
- Around line 258-271: copyFile currently defers in.Close() and out.Close()
which can swallow Close() errors (e.g., disk full after io.Copy); change
copyFile to capture and return any Close() errors by replacing the deferred
out.Close() and in.Close() with explicit close calls after io.Copy (or defer but
store their error values), e.g., call n, err := io.Copy(out, in); then closeErr
:= out.Close(); if err == nil { err = closeErr }; finally closeErr2 :=
in.Close(); if err == nil { err = closeErr2 }; return err so any failure from
out.Close() or in.Close() is propagated from copyFile.
---
Nitpick comments:
In `@pkg/config/types.go`:
- Around line 143-159: Add a doc comment to the ExpectedDepItemsForTarget method
explaining its behavior for unsupported/other target types: state that it
explicitly handles "tackle-hub" (returns DepItemsHub when DepFileHub is set) and
"kantra" (returns DepItemsKantra when DepFileKantra is set), and that the
default case falls back to kantra semantics (i.e., will only return items if
DepFileKantra is set), so targets like "tackle-ui", "kai-rpc", "vscode" will not
perform dependency validation unless DepFileKantra is present; reference the
method ExpectedDepItemsForTarget and the fields DepFileHub, DepItemsHub,
DepFileKantra, DepItemsKantra in the comment.
In `@pkg/targets/tackle_hub_attachments.go`:
- Around line 125-135: The defer os.RemoveAll(sub) inside the loop in
findDepsYAMLInTree causes all temp dirs created by os.MkdirTemp to remain until
the function returns; change the logic so each temporary directory is removed
immediately after it's no longer needed: create the temp dir (os.MkdirTemp),
call extractTarArchive and then call walkForDepsYAML on it, and if
walkForDepsYAML returns a path return it after removing the temp dir, otherwise
remove the temp dir immediately (or encapsulate the extract+inspect+cleanup into
a helper that ensures os.RemoveAll(sub) runs before continuing); reference
extractTarArchive, walkForDepsYAML and os.RemoveAll to locate and implement the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98fa4293-d5d0-4606-9c8e-16385c335164
📒 Files selected for processing (10)
Makefilepkg/cli/generate.gopkg/cli/run.gopkg/config/loader.gopkg/config/types.gopkg/targets/tackle_hub.gopkg/targets/tackle_hub_attachments.gotests/book-server-deps/expected-deps-hub.yamltests/book-server-deps/expected-deps-kantra.yamltests/book-server-deps/test.yaml
✅ Files skipped from review due to trivial changes (2)
- tests/book-server-deps/expected-deps-kantra.yaml
- tests/book-server-deps/expected-deps-hub.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/book-server-deps/test.yaml
- Makefile
- pkg/cli/run.go
- pkg/config/loader.go
Comment on lines
+258
to
+271
| func copyFile(src, dst string) error { | ||
| in, err := os.Open(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer in.Close() | ||
| out, err := os.Create(dst) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer out.Close() | ||
| _, err = io.Copy(out, in) | ||
| return err | ||
| } |
There was a problem hiding this comment.
copyFile may lose errors on Close().
If io.Copy succeeds but out.Close() fails (e.g., disk full during flush), the error is silently lost because defer out.Close() discards its return value.
🔧 Proposed fix
func copyFile(src, dst string) error {
in, err := os.Open(src)
if err != nil {
return err
}
defer in.Close()
out, err := os.Create(dst)
if err != nil {
return err
}
- defer out.Close()
- _, err = io.Copy(out, in)
- return err
+ _, copyErr := io.Copy(out, in)
+ closeErr := out.Close()
+ if copyErr != nil {
+ return copyErr
+ }
+ return closeErr
}📝 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.
Suggested change
| func copyFile(src, dst string) error { | |
| in, err := os.Open(src) | |
| if err != nil { | |
| return err | |
| } | |
| defer in.Close() | |
| out, err := os.Create(dst) | |
| if err != nil { | |
| return err | |
| } | |
| defer out.Close() | |
| _, err = io.Copy(out, in) | |
| return err | |
| } | |
| func copyFile(src, dst string) error { | |
| in, err := os.Open(src) | |
| if err != nil { | |
| return err | |
| } | |
| defer in.Close() | |
| out, err := os.Create(dst) | |
| if err != nil { | |
| return err | |
| } | |
| _, copyErr := io.Copy(out, in) | |
| closeErr := out.Close() | |
| if copyErr != nil { | |
| return copyErr | |
| } | |
| return closeErr | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/targets/tackle_hub_attachments.go` around lines 258 - 271, copyFile
currently defers in.Close() and out.Close() which can swallow Close() errors
(e.g., disk full after io.Copy); change copyFile to capture and return any
Close() errors by replacing the deferred out.Close() and in.Close() with
explicit close calls after io.Copy (or defer but store their error values),
e.g., call n, err := io.Copy(out, in); then closeErr := out.Close(); if err ==
nil { err = closeErr }; finally closeErr2 := in.Close(); if err == nil { err =
closeErr2 }; return err so any failure from out.Close() or in.Close() is
propagated from copyFile.
mguetta1
requested changes
May 4, 2026
| output: | ||
| file: expected-output.yaml | ||
| dep-file-kantra: expected-deps-kantra.yaml | ||
| dep-file-hub: expected-deps-hub.yaml |
Contributor
There was a problem hiding this comment.
Can we have a single source of truth to check dependencies?
I’m not aware of the differences, but if it’s only file paths, koncur generate should update the expected file to contain only the suffix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests