👻 Refactor of the dependency download and resolution system #922
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughRefactors the Java external provider into a pluggable BuildTool/Downloader/Resolver architecture, adds decompilation, indexing, caching, labeling, Gradle/Maven/binary resolvers, many new artifact handlers and tests, adjusts CI/Makefile/docs/examples, and removes legacy inlined utilities. (≈26 words) Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"actorBorder":"#2d6cdf","actorBackground":"#e8f0ff","noteBorder":"#2d6cdf","noteBackground":"#f3f8ff"}}}%%
sequenceDiagram
participant Provider as Java Provider
participant BuildTool as BuildTool (Maven/Gradle/Binary)
participant Cache as DepCache
participant Resolver as Resolver
participant Decompiler as Decompiler
Provider->>BuildTool: GetDependencies(ctx)
alt cache hit
BuildTool->>Cache: useCache()
Cache-->>BuildTool: deps (hit)
else cache miss
BuildTool->>BuildTool: run mvn/gradle/binary flow
BuildTool->>Cache: setCachedDeps(deps, err)
end
BuildTool-->>Provider: deps map
Provider->>Resolver: ResolveSources(ctx)
Resolver->>Decompiler: DecompileIntoProject / Decompile
Decompiler->>Decompiler: enqueue jobs -> worker pool (concurrent)
Decompiler-->>Resolver: JavaArtifacts (sources)
Resolver-->>Provider: sourceLocation, dependencyLocation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
jwmatthews
changed the title
pranavgaikwad
left a comment
pranavgaikwad
left a comment
There was a problem hiding this comment.
I think I largely like the long due refactor. Thanks for doing the hard work. I am going to look again at EAR case once some of my questions are clarified:
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/tmpfile
Outdated
Show resolved
Hide resolved
shawn-hurley
force-pushed
the
refactor/update-java-dependency
branch
3 times, most recently
from
9ae3f1e to
5a0fcf3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 56
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
external-providers/java-external-provider/main.go (1)
17-17: Inconsistent log level defaults and confusing logic flow.There's an inconsistency between the flag default at line 17 (value: 5) and the hardcoded value at line 31 (value: 30, though invalid). Additionally, the conditional logic at lines 38-40 checks against the flag default to determine whether to override the hardcoded value, which is a backwards approach.
This creates confusion and makes the code harder to maintain. Consider one of these approaches:
Option 1 (recommended): Remove the hardcoded level and rely on the flag default:
logrusLog := logrus.New() logrusLog.SetOutput(os.Stdout) logrusLog.SetFormatter(&logrus.TextFormatter{}) - logrusLog.SetLevel(logrus.Level(30)) + logrusLog.SetLevel(logrus.Level(*logLevel)) log := logrusr.New(logrusLog) log = log.WithName("java-provider") // must use lspServerName for use of multiple grpc providers client := java.NewJavaProvider(log, *lspServerName, *contextLines, provider.Config{}) - if logLevel != nil && *logLevel != 5 { - logrusLog.SetLevel(logrus.Level(*logLevel)) - }Option 2: Update the flag default to match the intended hardcoded value (after fixing the invalid value 30):
- logLevel = flag.Int("log-level", 5, "Level to log") + logLevel = flag.Int("log-level", 5, "Level to log (0-6: Panic, Fatal, Error, Warn, Info, Debug, Trace)")And simplify the conditional:
if logLevel != nil && *logLevel != 5 { logrusLog.SetLevel(logrus.Level(*logLevel)) }Also applies to: 31-31, 38-40
provider/provider.go (2)
897-903: Bug: wrong index used when recording appended dep position (can corrupt indices).
len(deduped) - 1uses the map size, not the slice length for this URI. This can point to the wrong element and later index panics.Apply this diff:
- if dep.Indirect { - depSeen[id+"indirect"] = intPtr(len(deduped) - 1) - } else { - depSeen[id+"direct"] = intPtr(len(deduped) - 1) - } + if dep.Indirect { + depSeen[id+"indirect"] = intPtr(len(deduped[uri]) - 1) + } else { + depSeen[id+"direct"] = intPtr(len(deduped[uri]) - 1) + }
257-263: Bug: variable shadowing breaks nested provider config conversion.Inner
new, err := ...shadows the outernewmap;new[s] = newassigns the inner map to itself and discards the result. Nested maps never reach the outer aggregate.Apply this diff:
- if o, ok := v.(map[interface{}]interface{}); ok { - new, err := validateUpdateInternalProviderConfig(o) + if o, ok := v.(map[interface{}]interface{}); ok { + child, err := validateUpdateInternalProviderConfig(o) if err != nil { return nil, err } - new[s] = new + new[s] = child continue }
♻️ Duplicate comments (3)
provider/provider.go (1)
692-695: Remove duplicate error log; let caller log the failure.The engine already logs condition evaluation errors. Logging here causes double-reporting without added value. Either remove this log or demote to a high verbosity debug log.
Apply this diff:
if err != nil { - log.Error(err, "unable to get regex for name search") return resp, err }external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go (1)
86-89: No-op jobCtx.Done() call.jobCtx.Done() returns a channel; calling it here has no effect. Remove for clarity.
- span.End() - jobCtx.Done() + span.End()external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (1)
115-127: Consider renaming ShouldResolve() → MustResolve() for intent clarity.Matches prior feedback and better reflects mandatory resolution for binaries.
[ suggest_optional_refactor ]
🧹 Nitpick comments (32)
.github/workflows/pr-testing.yml (1)
24-27: Useworking-directoryfor better GitHub Actions idiomaticity.The step logic is correct, but GitHub Actions conventionally uses
working-directoryinstead of inlinecdcommands for improved clarity and consistency.Apply this diff to use the idiomatic GitHub Actions pattern:
- name: Test Java External Provider - run: | - cd external-providers/java-external-provider/ - go test -v ./... + working-directory: external-providers/java-external-provider/ + run: go test -v ./...external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/App.java (1)
15-17: Test structure looks appropriate for incident detection validation.The conditional wrapping of the
Fileinstantiation enables the analyzer to detect and target theFileReader.fileExists()call, which aligns with the PR's incident targeting improvements.Note: The unused
filevariable and always-true condition appear intentional for this test case.Optionally, consider adding a comment to clarify the test's purpose:
+ // Test case: Validates analyzer detection of FileReader usage if (FileReader.fileExists()) { File file = new File("/test"); }engine/engine.go (1)
573-581: Avoid noisy/duplicate log when no snippet is available.
getCodeLocationalready logs; this adds a second info log for the same case. Silence it and only set CodeSnip when non-empty.Apply this diff:
- if err != nil { - r.logger.V(6).Error(err, "unable to get code location") - } else if codeSnip == "" { - r.logger.V(3).Info("no code snippet returned", "rule", rule) - } else { + if err != nil { + r.logger.V(6).Error(err, "unable to get code location") + } else if codeSnip != "" { incident.CodeSnip = codeSnip }external-providers/java-external-provider/pkg/java_external_provider/dependency/constants.go (1)
5-8: Add brief doc comments for exported constants (and consider clearer names).Public constants should have comments for godoc; optional: prefer
JavaDir/WebAppDirfor clarity.Apply this diff:
const ( - JAVA = "src/main/java" - WEBAPP = "src/main/webapp" + // JAVA is the relative source directory for Java code on non-Windows. + JAVA = "src/main/java" + // WEBAPP is the relative web resources directory on non-Windows. + WEBAPP = "src/main/webapp" )external-providers/java-external-provider/pkg/java_external_provider/dependency/constants_windows.go (1)
5-8: Mirror comments for Windows constants (keep API symmetry).Add godoc comments; keeps docs consistent across platforms.
Apply this diff:
const ( - JAVA = `src\main\java` - WEBAPP = `src\main\webapp` + // JAVA is the relative source directory for Java code on Windows. + JAVA = `src\main\java` + // WEBAPP is the relative web resources directory on Windows. + WEBAPP = `src\main\webapp` )external-providers/java-external-provider/pkg/java_external_provider/dependency/project_create_test.go (1)
92-98: Verbatim byte compare is brittle; normalize or parseExact byte equality will fail on benign formatting changes. Normalize whitespace or parse XML for semantic compare.
- if !bytes.Equal(pomContent, []byte(expectedPom)) { + if !bytes.Equal(bytes.TrimSpace(pomContent), bytes.TrimSpace([]byte(expectedPom))) {Or parse both with an XML lib and compare nodes.
external-providers/java-external-provider/pkg/java_external_provider/dependency/jar_explode.go (1)
114-118: Propagate cancellation: use j.ctx, not context.Background().
- Keeps decompile killable on cancellation/timeouts.
Already reflected in the diff above.
external-providers/java-external-provider/docs/architecture.md (2)
29-29: Add languages to fenced code blocks (MD040).Specify languages for all fences (e.g., go, bash, json, text, mermaid). This will satisfy markdownlint and improve readability.
Also applies to: 135-135, 190-190, 232-232, 324-324, 369-369, 444-444, 503-503, 532-532, 577-577, 613-613, 696-696, 821-821, 871-871, 894-894, 961-961, 1012-1012, 1039-1039, 1083-1083, 1193-1193, 1235-1235, 1443-1443, 1519-1519, 1573-1573, 1658-1658, 1671-1671, 1684-1684, 1702-1702
658-676: Fix typos and stale filenames (expload → explode; jar_expload.go → jar_explode.go).Update the terminology and file paths to match the codebase.
Also applies to: 670-676
external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go (1)
37-65: LGTM; minor nits only.
- Consider ensuring projectPath is created/clean before use, or make it unique (MkdirTemp) if parallel runs share a parent dir.
- settingsFile/insecure/cleanBin fields are unused here; if not needed, drop or wire them.
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go (1)
80-93: Prefer fatal assertions to stop on unrecoverable errors.Use t.Fatalf for mismatched locations/paths to avoid continuing after failure.
external-providers/java-external-provider/pkg/java_external_provider/bldtool/dep_cache.go (2)
19-31: Locking contract is fragile; document and enforce unlock.
- useCache() locks on miss but relies on caller to always call setCachedDeps; add a comment or return an unlock func.
- At minimum, ensure setCachedDeps always unlocks even if hashing fails.
I recommend returning (hit bool, unlock func(), err error) from useCache to make the contract explicit.
34-36: Return a copy to avoid external mutation of cached deps.getCachedDeps exposes the internal map; consider returning a shallow copy.
Can provide a small helper if desired.
external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go (1)
69-72: Guard Gradle version selection when version is unknown.If g.gradleVersion is unset, the comparison may misroute script selection. Consider defaulting to legacy task or parsing errors explicitly.
Apply:
gradle9version, _ := version.NewVersion("9.0") -if g.gradleVersion.GreaterThanOrEqual(gradle9version) { +if (g.gradleVersion.String() != "") && g.gradleVersion.GreaterThanOrEqual(gradle9version) { g.taskFile = filepath.Join(filepath.Dir(g.taskFile), "task-v9.gradle") }external-providers/java-external-provider/pkg/java_external_provider/dependency/decompiler_test.go (2)
499-505: Duplicate expected artifact (spring-aop) listed twice.Remove one occurrence to keep expectations deterministic.
{ FoundOnline: false, Packaging: ".jar", GroupId: "io.konveyor.embededdep", ArtifactId: "spring-aop-3.1.2", Version: "0.0.0-SNAPSHOT", }, - { - FoundOnline: false, - Packaging: ".jar", - GroupId: "io.konveyor.embededdep", - ArtifactId: "spring-aop-3.1.2", - Version: "0.0.0-SNAPSHOT", - },
584-605: Version mismatch vs mavenDir list.Expected migration-support is 1.1.0 but mavenDir map uses 1.0.0. Align to one.
- Version: "1.1.0", + Version: "1.0.0",external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go (1)
49-51: No-op jobCtx.Done() call.Remove for clarity; span end is sufficient.
- span.End() - jobCtx.Done() + span.End()external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go (2)
39-45: Include the invalid path in error and consider temp dir default.Returning a generic error message hides which path failed. Also, defaulting to “.” can pollute CWD; prefer a temp dir when no destPath is provided.
- if destPath != "" { - if stat, err := os.Stat(destPath); err != nil || !stat.IsDir() { - return "", fmt.Errorf("output path does not exist or not a directory") - } - outputDir = destPath - } + if destPath != "" { + if stat, err := os.Stat(destPath); err != nil || !stat.IsDir() { + return "", fmt.Errorf("output path %q does not exist or is not a directory", destPath) + } + outputDir = destPath + } + // Optionally: create a temp output dir when not provided. + // tmp, err := os.MkdirTemp("", "mvn-copy-*") + // if err == nil { outputDir = tmp }
36-38: Clarify coordinates format in error message.Maven expects group:artifact:version[:packaging[:classifier]]. Update the hint to reflect packaging/classifier rather than “classifier as 4th part”.
- return "", fmt.Errorf("invalid maven coordinates in location %s, must be in format mvn://<group>:<artifact>:<version>:<classifier>@<path>", m.location) + return "", fmt.Errorf("invalid maven coordinates in %s; expected mvn://<group>:<artifact>:<version>[:packaging[:classifier]][@<path>]", m.location)external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go (1)
203-226: Add timeout to ‘gradlew projects’.Without a timeout this can hang indefinitely on broken projects.
- cmd := exec.Command(exe, args...) + tctx, cancel := context.WithTimeout(ctx, 60*time.Second) + defer cancel() + cmd := exec.CommandContext(tctx, exe, args...)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go (2)
121-124: Use filepath.Join for cross-platform submodule paths.String concatenation with “/” breaks on Windows.
- mPath := fmt.Sprintf("%s/%s/%s", filepath.Dir(location), mod, dependency.PomXmlFile) + mPath := filepath.Join(filepath.Dir(location), mod, dependency.PomXmlFile)
146-163: Trim Maven local repo path.help:evaluate typically prints a trailing newline; trim to avoid “file://…\n” prefixes.
- return outb.String() + return strings.TrimSpace(outb.String())external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go (1)
53-55: Handle any Stat error, not just NotExist.If Stat fails (e.g., permission denied), we should not proceed.
- if _, err := os.Stat(f); errors.Is(err, os.ErrNotExist) { - return nil - } + if _, err := os.Stat(f); err != nil { + return nil + }external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
124-135: Solid gating of OSS libs with selector; minor log tweak.Logic is good; consider logging “labelSelector” as a single key without space to match conventions.
- p.log.Error(err, "could not construct dep label selector from condition context, search scope will not be limited", "label selector", condCTX.DepLabelSelector) + p.log.Error(err, "could not construct dep label selector; search scope not limited", "labelSelector", condCTX.DepLabelSelector)
379-387: Project/classpath bootstrap only when buildTool is nil is correct.This aligns with the new build-tool flow. Consider removing the unfinished TODO.
- //TODO: This needs to happen only when + // Bootstraps only when no supported build tool is detected.external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go (1)
138-175: Parser likely misses multi-line unresolved artifact lists from Maven output.The regex only runs on the warning line; Maven often lists GAVs on subsequent lines. Consider scanning until the block ends and matching each line.
Example approach:
- Set a flag when the warning line is seen.
- Collect following lines until an empty line or next “[INFO]/[WARNING]” header.
- Apply artifactRegex to each collected line.
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
252-257: Use file permissions for files, not directory perms.OpenFile uses DirPermRWX; prefer FilePermRW for pom.xml.
- pomFile, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_TRUNC|os.O_CREATE|os.O_WRONLY, DirPermRWX) + pomFile, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_TRUNC|os.O_CREATE|os.O_WRONLY, FilePermRW) @@ - pom, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_CREATE|os.O_WRONLY, DirPermRWX) + pom, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_CREATE|os.O_WRONLY, FilePermRW)Also applies to: 269-274
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (2)
178-201: Return scanner error after reading patterns.Avoid silently succeeding on partial reads.
- return depToLabelsItems, nil + if err := scanner.Err(); err != nil { + return nil, err + } + return depToLabelsItems, nil
208-213: Handle nil selector without returning a stale error.Ensure a clean nil error when selector construction succeeded.
- if selector == nil { - return false, err - } + if selector == nil { + return false, nil + }external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go (1)
219-224: Key uses path+name twice; use path directly for the file URI.Avoid duplicating the filename in the key.
- w.deps[uri.URI(filepath.Join(path, info.Name()))] = []provider.DepDAGItem{ + w.deps[uri.URI(path)] = []provider.DepDAGItem{external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (2)
210-214: Consider returning all errors or a wrapped error.Lines 210-214: Multiple errors may occur during decompilation, but only the first error is returned. Consider returning all errors wrapped together or using a multi-error type for better error reporting.
Example approach using error wrapping:
if len(errs) != 0 { if len(errs) == 1 { return artifacts, errs[0] } // Return multiple errors wrapped return artifacts, fmt.Errorf("multiple decompilation errors: %v", errs) }
264-266: Consider returning all errors or a wrapped error.Similar to the
Decompilemethod, only the first error is returned when multiple errors occur. Consider returning all errors for better debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/acmeair-common-1.0-SNAPSHOT.jaris excluded by!**/*.jarexternal-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/acmeair-webapp-1.0-SNAPSHOT.waris excluded by!**/*.warexternal-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/fernflower.jaris excluded by!**/*.jar
📒 Files selected for processing (42)
.github/workflows/pr-testing.yml(1 hunks)Makefile(1 hunks)demo-output.yaml(5 hunks)engine/engine.go(1 hunks)external-providers/java-external-provider/docs/architecture.md(1 hunks)external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/App.java(2 hunks)external-providers/java-external-provider/go.mod(2 hunks)external-providers/java-external-provider/main.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/dep_cache.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency.go(2 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/constants.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/constants_windows.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompiler_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/explosion.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/jar.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/jar_explode.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/project_create_test.go(2 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go(0 hunks)external-providers/java-external-provider/pkg/java_external_provider/filter.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(9 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider_test.go(0 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(4 hunks)external-providers/java-external-provider/pkg/java_external_provider/util.go(0 hunks)parser/rule_parser.go(1 hunks)provider/provider.go(1 hunks)rule-example.yaml(2 hunks)
💤 Files with no reviewable changes (3)
- external-providers/java-external-provider/pkg/java_external_provider/provider_test.go
- external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go
- external-providers/java-external-provider/pkg/java_external_provider/util.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
PR: konveyor/analyzer-lsp#859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/dependency.goexternal-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.goexternal-providers/java-external-provider/pkg/java_external_provider/dependency/project_create_test.goexternal-providers/java-external-provider/pkg/java_external_provider/provider.go
📚 Learning: 2025-08-21T12:42:54.499Z
Learnt from: jmle
PR: konveyor/analyzer-lsp#888
File: external-providers/java-external-provider/pkg/java_external_provider/service_client.go:507-521
Timestamp: 2025-08-21T12:42:54.499Z
Learning: In the konveyor/analyzer-lsp Java external provider (external-providers/java-external-provider/pkg/java_external_provider/service_client.go), the maintainers prefer to keep the current JAVA_HOME selection logic in GetJavaHomeForGradle method simple, as they operate in a controlled environment where both Java 8 and Java 17+ are guaranteed to be available, with Java 17+ being a system requirement for running the analyzer.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.goexternal-providers/java-external-provider/pkg/java_external_provider/provider.go
🧬 Code graph analysis (25)
external-providers/java-external-provider/main.go (1)
event/tag/tag.go (1)
Level(45-45)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (2)
external-providers/golang-dependency-provider/main.go (1)
GetDependenciesDAG(59-90)provider/provider.go (1)
DepDAGItem(475-475)
external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go (3)
tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (5)
DirPermRWXGrp(34-34)JavaArchive(18-18)WebArchive(19-19)METAINF(27-27)PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
CopyFile(146-165)
external-providers/java-external-provider/pkg/java_external_provider/dependency/jar_explode.go (5)
tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
DirPermRWXGrp(34-34)METAINF(27-27)PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/constants.go (1)
JAVA(6-6)external-providers/java-external-provider/pkg/java_external_provider/dependency/constants_windows.go (1)
JAVA(6-6)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
CopyFile(146-165)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)provider/provider.go (2)
DepDAGItem(475-475)Dep(474-474)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
PomXmlFile(23-23)
external-providers/java-external-provider/pkg/java_external_provider/dependency/jar.go (4)
tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (2)
ToDependency(56-79)JavaArtifact(24-31)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
EMBEDDED_KONVEYOR_GROUP(39-39)DecomplierResponse(94-98)DirPermRWXGrp(34-34)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
CopyFile(146-165)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go (2)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (1)
Downloader(38-40)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
MvnURIPrefix(22-22)
external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
ResolverOptions(74-124)Resolver(26-70)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (2)
DecompilerOpts(124-131)DefaultWorkerPoolSize(40-40)
external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go (5)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (4)
ResolverOptions(74-124)Resolver(26-70)CopyFile(146-165)AppendToFile(167-188)tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (2)
DecompilerOpts(124-131)DefaultWorkerPoolSize(40-40)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(24-31)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go (4)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (2)
BuildToolOptions(139-148)BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
Resolver(26-70)ResolverOptions(74-124)external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go (1)
GetGradleResolver(35-47)
external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go (5)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
ResolverOptions(74-124)Resolver(26-70)tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
DecompilerOpts(124-131)DefaultWorkerPoolSize(40-40)JavaArchive(18-18)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(24-31)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
CopyFile(146-165)ResolverOptions(74-124)external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go (1)
GetBinaryResolver(23-35)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)
external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go (3)
tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (4)
DirPermRWXGrp(34-34)WEBINF(28-28)METAINF(27-27)PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
CopyFile(146-165)
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompiler_test.go (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(24-31)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
DecompilerOpts(124-131)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (4)
Labeler(39-42)DepSourceLabel(26-26)JavaDepSourceOpenSource(17-17)JavaDepSourceInternal(16-16)engine/labels/labels.go (1)
AsString(35-40)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(24-31)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
DirPermRWX(33-33)FilePermRW(35-35)PomXmlFile(23-23)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (4)
provider/provider.go (4)
DepDAGItem(475-475)Config(87-96)InitConfig(121-152)Location(311-314)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
Resolver(26-70)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
JavaArchive(18-18)EnterpriseArchive(20-20)WebArchive(19-19)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go (6)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
Resolver(26-70)ResolverOptions(74-124)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (2)
BuildToolOptions(139-148)BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go (1)
GetBinaryResolver(23-35)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (3)
ToDependency(56-79)JavaArtifact(24-31)ToFilePathDependency(318-328)
external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/App.java (1)
external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/util/FileReader.java (1)
FileReader(5-10)
external-providers/java-external-provider/pkg/java_external_provider/dependency/project_create_test.go (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(24-31)
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(24-31)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (5)
provider/provider.go (4)
Location(311-314)InitConfig(121-152)Config(87-96)Dep(474-474)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (3)
ProviderSpecificConfigOpenSourceDepListKey(18-18)GetOpenSourceLabeler(53-61)Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go (1)
GetDownloader(16-21)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (2)
GetBuildTool(150-168)BuildToolOptions(139-148)provider/lib.go (1)
GetIncludedPathsFromConfig(380-403)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (1)
BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
CanRestrictSelector(203-213)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
engine/labels/labels.go (2)
AsString(35-40)NewLabelSelector(94-125)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go (5)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (2)
BuildToolOptions(139-148)BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
Resolver(26-70)ResolverOptions(74-124)external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go (1)
GetMavenResolver(31-42)
🪛 markdownlint-cli2 (0.18.1)
external-providers/java-external-provider/docs/architecture.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
190-190: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
324-324: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
444-444: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
503-503: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
532-532: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
577-577: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
696-696: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
821-821: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
871-871: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
894-894: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
961-961: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1012-1012: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1039-1039: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1083-1083: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1193-1193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1235-1235: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1443-1443: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1519-1519: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1573-1573: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1658-1658: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1671-1671: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1684-1684: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1702-1702: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (6)
- GitHub Check: test
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: benchmark (windows-latest, windows)
- GitHub Check: benchmark (macos-latest, mac)
🔇 Additional comments (28)
.github/workflows/pr-testing.yml (1)
21-27: Workflow structure looks good.The rename of the initial test step and addition of the Java provider test suite appropriately split the test responsibilities. Both steps inherit the matrix strategy (3 platforms), ensuring Java provider tests run on all supported environments—which aligns well with the PR's refactoring goals for the Java dependency resolution system.
parser/rule_parser.go (1)
815-815: LGTM! Helpful debug logging for dependency label selector.This debug log at V(9) provides useful transparency into when the dependency label selector is applied to providers, which aligns well with the PR's goal of improving visibility around dependency resolution workflows.
external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/App.java (1)
4-4: LGTM!The import addition supports the new FileReader-based conditional logic below.
Makefile (1)
91-91: The--dep-label-selectorflag is properly supported with full negation operator support.The label selector implementation in
engine/labels/labels.gosupports the negation operator (!) extensively, as demonstrated by comprehensive test cases inengine/labels/labels_test.go. The syntax!konveyor.io/dep-source=open-sourcematches well-tested patterns and will correctly exclude dependencies matching that label.The flag is properly defined in
cmd/analyzer/main.go:338, correctly parsed at line 102, and used for incident filtering inprovider/provider.go:567. The Makefile change is correct and will function as intended.external-providers/java-external-provider/go.mod (1)
10-10: Dependency marker adjustments align with refactoring scope and are verified.The changes correctly reflect the refactor's expanded build-tool abstraction:
- Line 10: Marking
go.opentelemetry.io/otelas indirect is correct—no direct imports exist in the provider code.- Line 36: Promoting
github.com/hashicorp/go-versionto direct is correct—the library is directly imported in three source files (gradle.go, gradle_resolver.go, and resolver.go).The go-version library is stable for parsing versions and version constraints, and no known security advisories exist for v1.6.0.
external-providers/java-external-provider/main.go (1)
33-33: Good addition for logging context.Adding a named scope to the logger improves log traceability and aligns well with the multi-provider architecture mentioned in the comment at line 35.
rule-example.yaml (2)
459-461: Clarify whether the nodejs pattern change is within PR scope.Line 461 changes the nodejs.referenced pattern to
"log". This change appears unrelated to the PR's Java dependency resolution refactor. Please confirm whether this is an intentional, unrelated fix or if it should be excluded from this PR.
280-284: Pattern change verified and working as intended.The updated pattern
io.konveyor.util.FileReadercorrectly targets the refactored FileReader class. The FileReader class exists atio.konveyor.util.FileReader, and the rule properly finds incidents inutil/FileReader.javawhen the includedPaths filter is applied, validating the test's intent for includedPaths configuration. The pattern change aligns with the Java provider refactoring.external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
16-19: LGTM: reuse DAG result to build flat deps list.Straightforward aggregation; matches the new DAG-first design.
If duplicates surface across branches in the DAG, ensure upstream deduplication is applied by the provider layer. Do you want a quick check added here to call
provider.deduplicateDependenciesbefore returning?external-providers/java-external-provider/docs/architecture.md (1)
602-606: Align decompilation command example with implementation.Docs show “-mpm=30”, but jar_explode.go uses “java -jar ” without flags. Either pass flags in code or adjust docs.
external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go (1)
96-101: Child process env overwrites PATH (gradlew likely breaks).cmd.Env with only JAVA_HOME drops PATH and other needed vars. Inherit env and append JAVA_HOME if set.
-cmd := exec.CommandContext(ctx, g.wrapper, args...) -cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", g.javaHome)) +cmd := exec.CommandContext(ctx, g.wrapper, args...) +cmd.Env = append(os.Environ(), + func() string { + if g.javaHome != "" { return fmt.Sprintf("JAVA_HOME=%s", g.javaHome) } + return "" + }(), +) +// filter out empty env entry +for i := 0; i < len(cmd.Env); { + if cmd.Env[i] == "" { cmd.Env = append(cmd.Env[:i], cmd.Env[i+1:]...); continue } + i++ +}⛔ Skipped due to learnings
Learnt from: jmle PR: konveyor/analyzer-lsp#888 File: external-providers/java-external-provider/pkg/java_external_provider/provider.go:662-663 Timestamp: 2025-08-21T12:39:46.778Z Learning: In the konveyor/analyzer-lsp Java provider, when setting JAVA_HOME for Gradle commands using exec.CommandContext, the maintainers prefer not to preserve the existing environment variables (including PATH) and use `cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))` instead of `cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))`.external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go (3)
377-391: JAVA_HOME selection matches maintainers’ preference.The simple JAVA8_HOME (<=8.14) vs JAVA_HOME (>8.14) logic aligns with prior guidance for controlled environments.
If you expect Gradle 9+ projects, confirm JAVA_HOME is a 17+ JDK at runtime.
182-186: Do not clobber environment; preserve PATH when setting JAVA_HOME.Setting cmd.Env without os.Environ() drops PATH and can break gradlew (shebang/env needs PATH). This affects command execution reliability.
- cmd := exec.CommandContext(timeout, exe, args...) + cmd := exec.CommandContext(timeout, exe, args...) cmd.Dir = filepath.Dir(g.hashFile) - cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome)) + cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))⛔ Skipped due to learnings
Learnt from: jmle PR: konveyor/analyzer-lsp#888 File: external-providers/java-external-provider/pkg/java_external_provider/provider.go:662-663 Timestamp: 2025-08-21T12:39:46.778Z Learning: In the konveyor/analyzer-lsp Java provider, when setting JAVA_HOME for Gradle commands using exec.CommandContext, the maintainers prefer not to preserve the existing environment variables (including PATH) and use `cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))` instead of `cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))`.
220-224: Same env issue in getGradleSubprojects.Preserve the existing environment when injecting JAVA_HOME.
- cmd := exec.Command(exe, args...) + cmd := exec.Command(exe, args...) cmd.Dir = filepath.Dir(g.hashFile) - cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome)) + cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))⛔ Skipped due to learnings
Learnt from: jmle PR: konveyor/analyzer-lsp#888 File: external-providers/java-external-provider/pkg/java_external_provider/provider.go:662-663 Timestamp: 2025-08-21T12:39:46.778Z Learning: In the konveyor/analyzer-lsp Java provider, when setting JAVA_HOME for Gradle commands using exec.CommandContext, the maintainers prefer not to preserve the existing environment variables (including PATH) and use `cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))` instead of `cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))`.external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go (3)
79-94: Source extraction fallback is reasonable.Defer decompilation until the dir is missing; unpacking with ‘jar xf’ adjacent to the JAR is fine and matches Maven layout.
145-154: Good timeout and error propagation for mvn dependency:tree.Batch mode, timeout, and verbose logging of output are appropriate.
22-25: Remove unused constant.mavenDepErr is declared but never used; this will fail compilation.
-const ( - mavenDepErr = "mvnErr" -) +// removed unused constant mavenDepErrLikely an incorrect or invalid review comment.
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go (2)
205-217: Note: ignoring errors from ToDependency/ToFilePathDependency is acceptable here.This follows prior guidance for dependency walker error tolerance.
Based on learnings
Also applies to: 236-248
74-86: Returning m (BuildTool implementing dependency.Resolver) is unconventional; verify intent.It works because mavenBinaryBuildTool implements ResolveSources and delegates to m.resolver, but returning m.resolver would be simpler and less surprising.
Do you want this method to return the inner resolver instead?
- m.resolver = dependency.GetBinaryResolver(opts) - return m, nil + m.resolver = dependency.GetBinaryResolver(opts) + return m.resolver, nilexternal-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (4)
16-41: LGTM!Constants are well-organized with clear documentation for permission values. The default worker pool size of 10 is reasonable for parallel decompilation.
351-393: LGTM!The internal decompilation methods correctly add jobs to the worker queue and manage the waitgroup for async processing.
466-480: LGTM!The worker implementation correctly handles job processing and graceful shutdown via context cancellation.
482-504: LGTM!Both utility functions are straightforward and correctly implement deduplication and filtering logic for Java artifacts.
external-providers/java-external-provider/pkg/java_external_provider/provider.go (5)
288-297: Proper config extraction for gradle and labels.The extraction of
gradleTaskFileandmavenIndexPathfrom the provider-specific config is correctly handled with type assertions and fallbacks.
302-309: Correct integration of remote artifact download logic.The
bldtool.GetDownloader()factory pattern correctly handles the optional download step before proceeding with build tool resolution. Error handling and cancellation are properly sequenced.
311-316: Proper error handling for open-source labeler initialization.The
labels.GetOpenSourceLabeler()call correctly returns an error on failure and cancels the context before returning.
17-18: New imports are available and accessible.Both packages verified:
bldtoolpackage exists with 7 Go fileslabelspackage exists with Go source fileNo issues found.
320-344: All required methods are properly defined and implemented.The verification confirms:
buildTool.ShouldResolve()exists on the BuildTool interface (bldtool/tool.go:126)buildTool.GetResolver(decompileTool string)exists on the BuildTool interface (bldtool/tool.go:113) and returns(dependency.Resolver, error)resolver.ResolveSources(ctx context.Context)exists on the Resolver interface (dependency/resolver.go) and returns(string string, error)All methods have implementations for Maven, Gradle, and binary build tools. The code usage in provider.go lines 320-344 is correct.
external-providers/java-external-provider/pkg/java_external_provider/bldtool/dep_cache.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/dep_cache.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
Show resolved
Hide resolved
...rnal-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/filter.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/service_client.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go
Outdated
Show resolved
Hide resolved
...nal-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Show resolved
Hide resolved
shawn-hurley
force-pushed
the
refactor/update-java-dependency
branch
from
5a0fcf3 to
0c778a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
provider/provider.go (2)
250-266: Fix map shadowing/self-reference in provider-specific config conversionInner variable ‘new’ shadows the outer map and assigns the map to itself, risking self-referential structures and panics on marshal.
func validateUpdateInternalProviderConfig(old map[interface{}]interface{}) (map[string]interface{}, error) { - new := map[string]interface{}{} + new := map[string]interface{}{} for k, v := range old { s, ok := k.(string) if !ok { return nil, fmt.Errorf("provider specific config must only have keys that strings") } if o, ok := v.(map[interface{}]interface{}); ok { - new, err := validateUpdateInternalProviderConfig(o) + child, err := validateUpdateInternalProviderConfig(o) if err != nil { return nil, err } - new[s] = new + new[s] = child continue }
896-902: Fix wrong index when tracking deduped dependenciesUses len(deduped) (map size) instead of current slice length; yields wrong indices and incorrect direct/indirect updates.
- deduped[uri] = append(deduped[uri], dep) - if dep.Indirect { - depSeen[id+"indirect"] = intPtr(len(deduped) - 1) - } else { - depSeen[id+"direct"] = intPtr(len(deduped) - 1) - } + deduped[uri] = append(deduped[uri], dep) + idx := len(deduped[uri]) - 1 + if dep.Indirect { + depSeen[id+"indirect"] = intPtr(idx) + } else { + depSeen[id+"direct"] = intPtr(idx) + }
♻️ Duplicate comments (41)
external-providers/java-external-provider/main.go (1)
31-31: Invalid log level value.This issue was already flagged in a previous review. Logrus level 30 is invalid (valid range is 0-6). Combined with the conditional logic at lines 38-40, this invalid level will be used when running with default configuration, causing undefined behavior.
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
33-33: Fix log typo and reduce verbosity.The typo "bldtooL" and the practice of dumping the entire tool struct at info level were already flagged in a previous review. Please apply the suggested fix to correct the typo and log the tool type at a lower verbosity.
provider/provider.go (1)
691-695: Avoid duplicate error logging on regex compile failureLet the caller decide logging; this layer should just return the error.
- log.Error(err, "unable to get regex for name search") return resp, errexternal-providers/java-external-provider/pkg/java_external_provider/filter.go (1)
213-216: Compile error: wrong type for second return valuegetURI returns (string, uri.URI, error); return uri.URI("") on error.
- if err != nil { - return "", "", err - } + if err != nil { + return "", uri.URI(""), err + }external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go (1)
64-69: Remove brittle filename inference; rely solely on Maven output path.This repeats a previously flagged issue: coordinates→filename mapping is wrong for classifiers/packaging. Parse the “Copying … to …” line and require it. Also trim quotes/spaces on the captured path.
Apply:
- downloadedPath := filepath.Join(outputDir, - fmt.Sprintf("%s.jar", strings.Join(mvnCoordinatesParts[1:3], "-"))) - if len(mvnCoordinatesParts) == 4 { - downloadedPath = filepath.Join(outputDir, - fmt.Sprintf("%s.%s", strings.Join(mvnCoordinatesParts[1:3], "-"), strings.ToLower(mvnCoordinatesParts[3]))) - } + downloadedPath := "" outputLinePattern := regexp.MustCompile(`.*?Copying.*?to (.*)`) for _, line := range strings.Split(string(mvnOutput), "\n") { if outputLinePattern.MatchString(line) { match := outputLinePattern.FindStringSubmatch(line) if match != nil { - downloadedPath = match[1] + downloadedPath = strings.Trim(match[1], ` "'`) } } } + if downloadedPath == "" { + return "", fmt.Errorf("unable to determine downloaded artifact path from mvn output for %q", mvnCoordinates) + }Also applies to: 70-78
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
105-142: Tighten POM parsing: reduce log noise, check scanner errors, return nil on success.Avoid Info logs per entry; verify
scanner.Err(); return nil error explicitly.- log.V(5).Info("trying to find pom within jar %s to get info", jarFile) + log.V(5).Info("trying to find pom within jar to get info", "jar", jarFile) @@ - log.Info("match", "match", match, "name", file.Name) + log.V(6).Info("jar entry", "match", match, "name", file.Name) @@ - for scanner.Scan() { + for scanner.Scan() { ... - } - return dep, err + } + if scanErr := scanner.Err(); scanErr != nil { + return dep, scanErr + } + return dep, nilexternal-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
147-149: Propagate the error from loadDepLabelItems.Currently the error is discarded and nil is returned unconditionally.
Apply this diff:
- items, err := loadDepLabelItems(file, labels.AsString(DepSourceLabel, JavaDepSourceOpenSource), nil) - return items, nil + items, err := loadDepLabelItems(file, labels.AsString(DepSourceLabel, JavaDepSourceOpenSource), nil) + return items, errexternal-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go (2)
189-194: Remove incorrect manual recursion in WalkDir callback.
filepath.WalkDiralready recurses into directories automatically. Manually callingWalkDiragain onfilepath.Join(path, info.Name())causes duplicated traversal and incorrect path construction.Apply this diff:
func (w *walker) walkDirForJar(path string, info fs.DirEntry, err error) error { if info == nil { return nil } if info.IsDir() { - return filepath.WalkDir(filepath.Join(path, info.Name()), w.walkDirForJar) + return nil }
259-264: Remove incorrect manual recursion in WalkDir callback.Same issue as
walkDirForJar:filepath.WalkDirrecurses automatically.Apply this diff:
func (w *walker) walkDirForPom(path string, info fs.DirEntry, err error) error { if info == nil { return nil } if info.IsDir() { - return filepath.WalkDir(filepath.Join(path, info.Name()), w.walkDirForPom) + return nil }external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (9)
94-98: Fix typos in struct name and field.
DecomplierResponseshould beDecompilerResponseouputLocationBaseshould beoutputLocationBaseApply this diff:
-type DecomplierResponse struct { +type DecompilerResponse struct { Artifacts []JavaArtifact - ouputLocationBase string + outputLocationBase string err error }
124-131: Fix typo in field name.Line 128:
lablershould belabeler.Apply this diff:
type DecompilerOpts struct { DecompileTool string log logr.Logger workers int - labler labels.Labeler + labeler labels.Labeler m2Repo string mavenIndexPath string }
133-155: Add cleanup method to prevent goroutine leak.The decompiler starts worker goroutines but
cancelWorkersFuncis never called, leading to goroutine leaks.Consider adding a
Close()orShutdown()method to the Decompiler interface and implementing it to call the saved cancel function and close the jobs channel.
148-148: Fix typo in variable name.Line 148:
workerCacnelFuncshould beworkerCancelFunc.
183-208: Remove incorrect Done() call in receiver goroutine.Line 197:
job.decompilerWG.Done()is called inside the receiver goroutine, butjob.Run()already callsDone()via defer (jar.go:19). Calling it again will cause a panic.Remove the Done() call at line 197—the receiver should only collect responses, not manage the waitgroup.
186-186: Remove debug log statement.Line 186:
d.log.Info("here")appears to be a debug statement that should be removed or made more descriptive.
271-346: Significant code duplication in job initialization.The three cases for
JavaArchive,WebArchive, andEnterpriseArchiveduplicate the samebaseArtifactconstruction.Extract a helper method to create the baseArtifact to reduce duplication and improve maintainability.
395-464: Code duplication with getIntoProjectJob.This method has the same duplication issue as
getIntoProjectJob.
462-462: Fix typo in error message.Line 462:
"unable to get a job fo rthe artifact"should be"unable to get a job for the artifact".external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go (1)
74-77: Remove incorrect error check.
GetBinaryResolverreturns aResolver, not an error. Theerrvariable here refers to theCopyFilecall at line 58, which was already checked at lines 59-61.Apply this diff:
resolver := GetBinaryResolver(ResolverOptions{ Log: testr.NewWithOptions(t, testr.Options{ Verbosity: 2, }), Location: filepath.Clean(newLocation), DecompileTool: fernflower, Labeler: &testLabeler{}, LocalRepo: filepath.Clean(mavenDir), Insecure: false, MavenIndexPath: "test", }) - if err != nil { - t.Logf("unable to get resolver: %s", err) - t.Fail() - }external-providers/java-external-provider/pkg/java_external_provider/dependency/explosion.go (2)
23-27: Use MkdirTemp and handle errors.Using
rand.IntN(100)for temp directory naming has low entropy and risks collisions. TheMkdirAllerror is also ignored.Prefer
os.MkdirTempto create a unique temp directory and check for errors.
29-35: Harden archive extraction against Zip Slip.Spawning the external
jartool can fail if unavailable, and extraction may write outsidetmpDirif archive paths are malicious.Consider using Go's
archive/zipwith path sanitization (reject entries containing ".." or absolute paths) or validate entries before extraction.external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go (3)
35-47: Wire all ResolverOptions fields.
opts.DisableMavenSearchandopts.LocalRepoare not assigned to the returnedgradleResolver, causing downstream components to use default/zero values.
114-179: Fix broken concurrency pattern.Workers never call
wg.Done, the aggregator defersDoneincorrectly, there's nowg.Wait,cancelFuncis called prematurely, and the unbuffered channel can block senders.Replace with a simple wg+mutex pattern as suggested in the past review comment.
248-271: Fix parsing bug and prevent out-of-bounds panic.Line 259: uses
match[4]instead ofgav[4], and Line 261: assignsmatch[4]which is out of bounds. No length checks on regex groups.Anchor regexes, add length checks, and use correct variable indices.
external-providers/java-external-provider/pkg/java_external_provider/dependency/jar.go (4)
20-21: Defer span.End(); remove no-op Done().Ensure span always ends.
- jobCtx, span := tracing.StartNewSpan(ctx, "java-artifact-job") + jobCtx, span := tracing.StartNewSpan(ctx, "java-artifact-job") + defer span.End() @@ - span.End() - jobCtx.Done()Also applies to: 91-93
47-52: Preserve discovered artifact on sources cache-hit.Don’t emit empty slice; consumers expect the artifact.
- j.decompilerResponses <- DecomplierResponse{ - Artifacts: []JavaArtifact{}, + j.decompilerResponses <- DecomplierResponse{ + Artifacts: artifacts, ouputLocationBase: filepath.Base(sourceDestPath), err: nil, }
58-75: Fix log typos/levels and avoid err shadowing.Clarify messages; use log.Error on failures; no := over err.
- log.Info("getting sources - allready found", "souce-dst", sourceDestPath, "destPath", destinationPath) + log.Info("decompiling sources", "source-dst", sourceDestPath, "destPath", destinationPath) if err = os.MkdirAll(destinationPath, DirPermRWXGrp); err != nil { - log.Info("getting sources - can not create dir", "souce-dst", sourceDestPath, "destPath", destinationPath) + log.Error(err, "failed to create destination dir", "source-dst", sourceDestPath, "destPath", destinationPath) return err } cmd := j.getDecompileCommand(jobCtx, j.artifactPath, destinationPath) - log.Info("getting sources - allready found", "souce-dst", sourceDestPath, "destPath", destinationPath, "cmd", cmd) - err := cmd.Run() - if err != nil { + log.Info("running decompiler", "source-dst", sourceDestPath, "destPath", destinationPath, "cmd", cmd) + if err := cmd.Run(); err != nil { log.Error(err, "failed to decompile file", "file", j.artifactPath) return err } - err = j.renameSourcesJar(destinationPath, sourceDestPath) - if err != nil { - log.Info("getting sources rename failure", "souce-dst", sourceDestPath, "destPath", destinationPath, "cmd", cmd, "err", err) - return err - } + if err := j.renameSourcesJar(destinationPath, sourceDestPath); err != nil { + log.Error(err, "failed to move sources jar", "source-dst", sourceDestPath, "destPath", destinationPath, "cmd", cmd) + return err + }
21-39: Don’t abort on ToDependency(); fallback to dummy artifact.Keeps pipeline robust when lookup fails. Packaging should be "jar".
Based on learnings
- dep, err := ToDependency(ctx, log, j.labeler, j.artifactPath, j.mavenIndexPath) - if err != nil { - log.Error(err, "failed to add dep", "file", j.artifactPath) - return err - } + dep, err := ToDependency(ctx, log, j.labeler, j.artifactPath, j.mavenIndexPath) + if err != nil { + log.Error(err, "unable to derive dependency; falling back to dummy", "file", j.artifactPath) + name := j.getFileName() + dep = JavaArtifact{ + FoundOnline: false, + Packaging: "jar", + GroupId: EMBEDDED_KONVEYOR_GROUP, + ArtifactId: name, + Version: "0.0.0-SNAPSHOT", + Sha1: "", + } + } @@ if !dep.IsValid() { log.Info("failed to create maven coordinates -- using file to create dummy values", "file", j.artifactPath) name := j.getFileName() newDep := JavaArtifact{ FoundOnline: false, - Packaging: "", + Packaging: "jar", GroupId: EMBEDDED_KONVEYOR_GROUP, ArtifactId: name, Version: "0.0.0-SNAPSHOT", Sha1: "", } dep = newDep }external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go (2)
76-83: Use artifact/request context for decompile, not Background.Propagate cancellation/timeouts.
- decompileCommand := exec.CommandContext(context.Background(), w.javaPath, "-jar", w.decompileTool, absPath, outputPath) + decompileCommand := exec.CommandContext(w.ctx, w.javaPath, "-jar", w.decompileTool, absPath, outputPath)
94-97: Create the actual parent directory, not “.”.filepath.Dir(filepath.Base(outputPath)) resolves to ".", breaking copy.
- if err = os.MkdirAll(filepath.Dir(filepath.Base(outputPath)), DirPermRWXGrp); err != nil { + if err = os.MkdirAll(filepath.Dir(outputPath), DirPermRWXGrp); err != nil {external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
115-122: Guard against nil buildTool before GetLocalRepoPath().Prevents panic when no tool detected.
- "includeOpenSourceLibraries": true, - "mavenLocalRepo": p.buildTool.GetLocalRepoPath(), + "includeOpenSourceLibraries": true, + "mavenLocalRepo": func() string { + if p.buildTool != nil { + return p.buildTool.GetLocalRepoPath() + } + return "" + }(),external-providers/java-external-provider/pkg/java_external_provider/dependency/decompiler_test.go (3)
449-456: Fix artifactId typo.“commons-loging” → “commons-logging”.
- ArtifactId: "commons-loging", + ArtifactId: "commons-logging",
556-575: Fix GroupId typos (wasdeb → wasdev).Align with real coordinates.
- GroupId: "net/wasdeb.wlp.sample", + GroupId: "net/wasdev.wlp.sample", @@ - GroupId: "net/wasdeb.wlp.sample", + GroupId: "net/wasdev.wlp.sample", @@ - GroupId: "net/wasdeb.wlp.sample", + GroupId: "net/wasdev.wlp.sample",
654-657: Fix inverted assertion; don’t mask failures.Use OR and negate DeepEqual.
- if len(test.artifacts) != len(artifacts) && reflect.DeepEqual(test.artifacts, artifacts) { + if len(test.artifacts) != len(artifacts) || !reflect.DeepEqual(test.artifacts, artifacts) {external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go (2)
108-115: Use artifact/request context for decompile.Enable cancellation/timeout.
- decompileCommand := exec.CommandContext(context.Background(), e.javaPath, "-jar", e.decompileTool, absPath, outputPath) + decompileCommand := exec.CommandContext(e.ctx, e.javaPath, "-jar", e.decompileTool, absPath, outputPath)
122-127: mkdir the directory itself, not its parent.Avoid missing target dir.
- if d.IsDir() { - if err = os.MkdirAll(filepath.Dir(outputPath), DirPermRWXGrp); err != nil { + if d.IsDir() { + if err = os.MkdirAll(outputPath, DirPermRWXGrp); err != nil { return err } return nil }external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go (1)
87-128: Fix deadlock-prone concurrency and JAR path; capture loop var; log correct error.Buffered results, wait/close properly, include version in path, and use resp.err.
- wg := &sync.WaitGroup{} - dependencies := []JavaArtifact{} - returnChan := make(chan struct { - artifact []JavaArtifact - err error - }) - decompilerCtx, cancelFunc := context.WithCancel(ctx) - - go func() { - for { - select { - case resp := <-returnChan: - defer wg.Done() - if resp.err != nil { - m.log.Error(err, "unable to get java artifact") - continue - } - dependencies = append(dependencies, resp.artifact...) - case <-decompilerCtx.Done(): - return - } - } - }() - for _, artifact := range artifacts { - m.log.WithValues("artifact", artifact).Info("sources for artifact not found, decompiling...") - - groupDirs := filepath.Join(strings.Split(artifact.GroupId, ".")...) - jarName := fmt.Sprintf("%s-%s.jar", artifact.ArtifactId, artifact.Version) - wg.Add(1) - go func() { - artifact, err := decompiler.Decompile(decompilerCtx, filepath.Join(m.localRepo, groupDirs, artifact.ArtifactId, jarName)) - returnChan <- struct { - artifact []JavaArtifact - err error - }{artifact: artifact, err: err} - }() - } - wg.Done() - cancelFunc() - - return m.location, m.localRepo, nil + wg := &sync.WaitGroup{} + dependencies := []JavaArtifact{} + results := make(chan struct { + artifact []JavaArtifact + err error + }, len(artifacts)) + decompilerCtx, cancelFunc := context.WithCancel(ctx) + defer cancelFunc() + + for _, a := range artifacts { + a := a // capture + m.log.WithValues("artifact", a).Info("sources for artifact not found, decompiling...") + groupDirs := filepath.Join(strings.Split(a.GroupId, ".")...) + jarName := fmt.Sprintf("%s-%s.jar", a.ArtifactId, a.Version) + jarPath := filepath.Join(m.localRepo, groupDirs, a.ArtifactId, a.Version, jarName) + wg.Add(1) + go func(j string) { + defer wg.Done() + arts, err := decompiler.Decompile(decompilerCtx, j) + results <- struct { + artifact []JavaArtifact + err error + }{artifact: arts, err: err} + }(jarPath) + } + + go func() { + wg.Wait() + close(results) + }() + + for resp := range results { + if resp.err != nil { + m.log.Error(resp.err, "unable to get java artifact") + continue + } + dependencies = append(dependencies, resp.artifact...) + } + + return m.location, m.localRepo, nilAlso applies to: 101-103, 113-118
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go (2)
105-135: Fix source file resolution: no param shadowing, correct root, unpack in jar dir.Avoids wrong paths and confusion.
-func (g *gradleBuildTool) GetSourceFileLocation(path string, jarPath string, javaFileName string) (string, error) { - sourcesFile := "" - jarFile := filepath.Base(jarPath) - walker := func(path string, d os.DirEntry, err error) error { - if err != nil { - return fmt.Errorf("found error traversing files: %w", err) - } - if !d.IsDir() && d.Name() == jarFile { - sourcesFile = path - return nil - } - return nil - } - root := filepath.Join(jarPath, "..", "..") - err := filepath.WalkDir(root, walker) - if err != nil { - return "", err - } - javaFileAbsolutePath := filepath.Join(filepath.Dir(sourcesFile), filepath.Dir(path), javaFileName) - - if _, err := os.Stat(filepath.Dir(javaFileAbsolutePath)); err != nil { - cmd := exec.Command("jar", "xf", filepath.Base(sourcesFile)) - cmd.Dir = filepath.Dir(sourcesFile) - err = cmd.Run() - if err != nil { - g.log.Error(err, "error unpacking java archive") - return "", err - } - } - return javaFileAbsolutePath, nil +func (g *gradleBuildTool) GetSourceFileLocation(packagePath string, jarPath string, javaFileName string) (string, error) { + dir := filepath.Dir(jarPath) + pkgDir := filepath.FromSlash(packagePath) + javaFileAbsolutePath := filepath.Join(dir, pkgDir, javaFileName) + + if _, err := os.Stat(filepath.Dir(javaFileAbsolutePath)); err != nil { + cmd := exec.Command("jar", "xf", filepath.Base(jarPath)) + cmd.Dir = dir + if err := cmd.Run(); err != nil { + g.log.Error(err, "error unpacking java archive") + return "", err + } + } + return javaFileAbsolutePath, nil
273-279: Guard depth/parent to avoid nil deref in dependency tree.Handle malformed indentation.
- dep.Dep.Indirect = true - depth := len(match[1]) / 5 // get the level of anidation of the dependency within the tree - parent := lastFoundWithDepth[depth-1] // find its parent - parent.AddedDeps = append(parent.AddedDeps, dep) // add child to parent - lastFoundWithDepth[depth] = &parent.AddedDeps[len(parent.AddedDeps)-1] // update last found with given depth + dep.Dep.Indirect = true + depth := len(match[1]) / 5 + if depth < 1 { + depth = 1 + } + parent, ok := lastFoundWithDepth[depth-1] + if !ok || parent == nil { + g.log.V(5).Info("skipping gradle dep with missing parent at depth") + continue + } + parent.AddedDeps = append(parent.AddedDeps, dep) + lastFoundWithDepth[depth] = &parent.AddedDeps[len(parent.AddedDeps)-1]external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
164-185: AppendToFile fails if destination doesn't exist; improve error wrapping.This issue was previously flagged and remains unresolved. The function will fail when the destination file doesn't exist because
os.OpenFileis called without theO_CREATEflag. Additionally, error messages should use%winstead of%sto preserve the error chain for better debugging.Apply this diff to fix both issues:
func AppendToFile(src string, dst string) error { // Read the contents of the source file content, err := os.ReadFile(src) if err != nil { - return fmt.Errorf("error reading source file: %s", err) + return fmt.Errorf("error reading source file: %w", err) } // Open the destination file in append mode - destFile, err := os.OpenFile(dst, os.O_APPEND|os.O_WRONLY, FilePermRW) + destFile, err := os.OpenFile(dst, os.O_APPEND|os.O_WRONLY|os.O_CREATE, FilePermRW) if err != nil { - return fmt.Errorf("error opening destination file: %s", err) + return fmt.Errorf("error opening destination file: %w", err) } defer destFile.Close() // Append the content to the destination file _, err = destFile.Write(content) if err != nil { - return fmt.Errorf("error appending to destination file: %s", err) + return fmt.Errorf("error appending to destination file: %w", err) } return nil }
238-246: Dependency merge exits loop on first match instead of continuing.This issue was previously flagged and remains unresolved. When a matching dependency is found, the code uses
breakwhich exits the entire loop, preventing remaining dependencies from being processed. The correct behavior is to usecontinueto skip adding the matched dependency and proceed to check the next one.Apply this diff to fix the logic:
for _, artifact := range dependencies { var found bool if slices.ContainsFunc(*pom.Dependencies, artifact.EqualsPomDep) { found = true } if found { - break + continue } foundUpdates = true *pom.Dependencies = append(*pom.Dependencies, artifact.ToPomDep()) }
🧹 Nitpick comments (19)
external-providers/java-external-provider/pkg/java_external_provider/dependency/constants_windows.go (1)
5-8: Consider usingfilepath.Joinfor better maintainability.The hardcoded Windows paths are functional, but using
filepath.Join("src", "main", "java")andfilepath.Join("src", "main", "webapp")would improve maintainability and make it clearer that these are filesystem paths.If you'd like to refactor, you could change the constants.go files to use
filepath.Join:+import "path/filepath" + const ( - JAVA = `src\main\java` - WEBAPP = `src\main\webapp` + JAVA = filepath.Join("src", "main", "java") + WEBAPP = filepath.Join("src", "main", "webapp") )Note: This would require the same change in both the Windows and non-Windows versions of the file, and the constants would become variables. Alternatively, you could eliminate platform-specific files entirely since
filepath.Joinhandles platform differences automatically.engine/engine.go (1)
573-579: LGTM! Improved error handling and logging clarity.The separation of error handling (line 573-574) from empty result handling (line 575-576) improves code clarity and observability. Only assigning
CodeSnipwhen there's actual content (line 578) is correct.Optional: Consider log level prioritization.
The error case logs at V(6) while the empty snippet case logs at V(3). Depending on your debugging needs, you might want actual errors to be more visible than informational empty results:
if err != nil { - r.logger.V(6).Error(err, "unable to get code location") + r.logger.V(5).Error(err, "unable to get code location", "rule", rule.RuleID, "uri", m.FileURI) } else if codeSnip == "" { - r.logger.V(3).Info("no code snippet returned", "rule", rule) + r.logger.V(6).Info("no code snippet returned", "rule", rule.RuleID, "uri", m.FileURI)This would make actual errors more prominent and add context for easier debugging.
parser/rule_parser.go (1)
815-817: Log the selector expression, not the struct pointerReduces log noise and stabilizes output at V(9).
- r.Log.V(9).Info("setting dependency label selector for provider", "language", langProvider, "selector", r.DepLabelSelector) + r.Log.V(9).Info("setting dependency label selector for provider", "language", langProvider, "selector", r.DepLabelSelector.Expression())provider/provider.go (1)
537-538: Tidy WithValues pairs and log condition as stringCurrent call pairs "provider info" with literal "cap". Use proper key/values and stringify bytes.
- log = log.WithValues("provider info", "cap", p.Capability, "condInfo", serializedInfo, "ruleID", condCtx.RuleID) + log = log.WithValues("cap", p.Capability, "condInfo", string(serializedInfo), "ruleID", condCtx.RuleID).github/workflows/pr-testing.yml (2)
16-22: Use latest setup-go and minor version rangeModernize action and float patch updates.
- - name: Set up Go - uses: actions/setup-go@v3 - with: - go-version: '1.23.9' + - name: Set up Go + uses: actions/setup-go@v5 + with: + go-version: '1.23.x'
24-28: Optional: avoid duplicate test runsIf root go test already covers submodules, this extra step may be redundant; keep if go.work is absent in CI.
Would you like me to check CI logs to confirm coverage overlap and prune this step accordingly?
external-providers/java-external-provider/docs/architecture.md (2)
29-29: Add language identifiers to fenced code blocks.markdownlint MD040 is flagging unlabeled fences. Please tag them:
- ASCII diagrams: use "text"
- Shell: "bash"
- Go: "go"
- XML/POM: "xml"
- URIs/examples: "text"
This improves readability and satisfies linters.
Also applies to: 135-135, 190-190, 232-232, 324-324, 369-369, 444-444, 503-503, 532-532, 577-577, 613-613, 696-696, 821-821, 871-871, 894-894, 961-961, 1012-1012, 1039-1039, 1083-1083, 1193-1193, 1235-1235, 1443-1443, 1519-1519, 1573-1573, 1658-1658, 1671-1671, 1684-1684, 1702-1702
1362-1370: Align example code with actual API (field names/export).Samples show lower‑case/unexported fields (“log”, “workers”, “labler”) and a function “getDecompiler”. If these are not the real exported identifiers, mark samples as pseudo‑code or fix names to compile against the current API.
Also applies to: 1311-1317
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go (1)
216-246: Maven dep string parsing: don’t treat parts[5] as “type”.For 6 fields Maven prints classifier and scope; parts[5] is scope, not packaging. Since Type isn’t used for path resolution, avoid storing scope in Type to reduce confusion. Also consider robust parsing against known formats.
Minimal tweak:
-} else if len(parts) == 6 { - d.Classifier = parts[3] - d.Version = parts[4] - d.Type = parts[5] +} else if len(parts) == 6 { + d.Classifier = parts[3] + d.Version = parts[4] + // parts[5] is scope; ignore or store separately if needed.external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (2)
247-256: Windows path normalization for GroupId.Use filepath.ToSlash before replacing “/” to handle “\”.
- dep.GroupId = strings.ReplaceAll(filepath.Dir(dir), "/", ".") + dep.GroupId = strings.ReplaceAll(filepath.ToSlash(filepath.Dir(dir)), "/", ".")
423-432: Bounds-check Maven index value before indexing.Guard against malformed lines.
- parts := strings.Split(str, ":") - dep.GroupId = parts[0] - dep.ArtifactId = parts[1] - dep.Version = parts[4] + parts := strings.Split(str, ":") + if len(parts) < 5 { + return dep + } + dep.GroupId = parts[0] + dep.ArtifactId = parts[1] + dep.Version = parts[4]external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
203-213: Simplify nil check logic.If
NewLabelSelectorreturns an error, the selector will be nil. The subsequent nil check at line 208-209 is redundant—the error check at line 205-207 already handles failure.Apply this diff:
func CanRestrictSelector(depLabelSelector string) (bool, error) { selector, err := labels.NewLabelSelector[*openSourceLabels](depLabelSelector, nil) if err != nil { return false, err } - if selector == nil { - return false, err - } matcher := openSourceLabels(true) return selector.Matches(&matcher) }external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact_test.go (1)
44-58: Add descriptive failure messages to test assertions.All
t.Fail()calls lack descriptive messages, making test failures harder to debug. Consider usingt.Errorfort.Fatalfwith clear messages about what went wrong.Apply this diff:
t.Run(tc.name, func(t *testing.T) { val, err := constructArtifactFromSHA(log, tc.jarFile, tc.mavenIndexPath) if err != nil && !tc.shouldFind { return } if err != nil { - t.Fail() + t.Fatalf("unexpected error: %v", err) } if !tc.shouldFind { - t.Fail() + t.Fatalf("expected not to find artifact but found: %+v", val) } if !reflect.DeepEqual(val, tc.value) { - t.Fail() + t.Errorf("artifact mismatch:\nexpected: %+v\ngot: %+v", tc.value, val) } })external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go (1)
79-105: Prefer t.Fatalf for clearer test failures.The pattern of
t.Logffollowed byt.Fail()can be simplified tot.Fatalffor clearer failure messages and immediate test termination.Apply this diff:
location, depPath, err := resolver.ResolveSources(context.Background()) if err != nil { - t.Logf("unable to resolve source: %s", err) - t.Fail() + t.Fatalf("unable to resolve source: %s", err) } if location != projectDir { - t.Logf("unable to get ExpectedLocation\nexpected: %s\nactual: %s", projectDir, location) - t.Fail() + t.Fatalf("location mismatch\nexpected: %s\nactual: %s", projectDir, location) } if depPath != mavenDir { - t.Logf("unable to get ExpectedLocalRepo\nexpected: %s\nactual: %s", mavenDir, depPath) - t.Fail() + t.Fatalf("depPath mismatch\nexpected: %s\nactual: %s", mavenDir, depPath) } test.testProject.matchProject(projectDir, t) missed := test.testProject.foundAllFiles() if len(missed) > 0 { - t.Logf("missed: %#v", missed) - t.Fail() + t.Fatalf("missed project files: %#v", missed) } test.mavenDir.matchMavenDir(mavenDir, t) missed = test.mavenDir.foundAllFiles() if len(missed) > 0 { - t.Logf("missed: %#v", missed) - t.Fail() + t.Fatalf("missed maven dir files: %#v", missed) }external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go (1)
36-37: Fix span lifecycle; remove no-op Done() and unused ctx.Defer End(); don’t hold an unused jobCtx.
- jobCtx, span := tracing.StartNewSpan(ctx, "war-artifact-job") + _, span := tracing.StartNewSpan(ctx, "war-artifact-job") + defer span.End() @@ - span.End() - jobCtx.Done()Also applies to: 49-51
external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go (2)
34-35: Fix span lifecycle; remove no-op Done() and unused ctx var.Defer End(); don’t keep jobCtx.
- jobCtx, span := tracing.StartNewSpan(ctx, "ear-artifact-job") + _, span := tracing.StartNewSpan(ctx, "ear-artifact-job") + defer span.End() @@ - span.End() - jobCtx.Done()Also applies to: 86-88
53-54: Remove or use accumulated errs.errs is never read; either aggregate-return or drop collection.
Would you like a small change to log and continue without errs?
Also applies to: 67-83
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go (1)
200-223: Use context with timeout when listing subprojects.Prevents hangs.
-func (g *gradleBuildTool) getGradleSubprojects(ctx context.Context) ([]string, error) { +func (g *gradleBuildTool) getGradleSubprojects(ctx context.Context) ([]string, error) { @@ - cmd := exec.Command(exe, args...) + timeout, cancel := context.WithTimeout(ctx, 2*time.Minute) + defer cancel() + cmd := exec.CommandContext(timeout, exe, args...)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
120-120: Add documentation for MavenIndexPath field.The
MavenIndexPathfield lacks a documentation comment explaining its purpose and usage, unlike the other fields in the struct.Apply this diff to add documentation:
+ // MavenIndexPath is the path to the Maven index directory for dependency lookups. + // Used by Maven-based resolvers for faster dependency resolution. MavenIndexPath string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
external-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/acmeair-common-1.0-SNAPSHOT.jaris excluded by!**/*.jarexternal-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/acmeair-webapp-1.0-SNAPSHOT.waris excluded by!**/*.warexternal-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/fernflower.jaris excluded by!**/*.jarexternal-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/should_find_in_index.jaris excluded by!**/*.jarexternal-providers/java-external-provider/pkg/java_external_provider/dependency/testdata/will_not_find.jaris excluded by!**/*.jar
📒 Files selected for processing (46)
.github/workflows/pr-testing.yml(1 hunks).gitignore(1 hunks)Makefile(1 hunks)demo-output.yaml(5 hunks)engine/engine.go(1 hunks)external-providers/generic-external-provider/Dockerfile(1 hunks)external-providers/java-external-provider/docs/architecture.md(1 hunks)external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/App.java(2 hunks)external-providers/java-external-provider/go.mod(2 hunks)external-providers/java-external-provider/main.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/dep_cache.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency.go(2 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact_bench_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/constants.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/constants_windows.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompiler_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/explosion.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/jar.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/jar_explode.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/project_create_test.go(2 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go(0 hunks)external-providers/java-external-provider/pkg/java_external_provider/filter.go(1 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider.go(10 hunks)external-providers/java-external-provider/pkg/java_external_provider/provider_test.go(0 hunks)external-providers/java-external-provider/pkg/java_external_provider/service_client.go(8 hunks)external-providers/java-external-provider/pkg/java_external_provider/util.go(0 hunks)parser/rule_parser.go(1 hunks)provider/provider.go(1 hunks)rule-example.yaml(2 hunks)
💤 Files with no reviewable changes (3)
- external-providers/java-external-provider/pkg/java_external_provider/util.go
- external-providers/java-external-provider/pkg/java_external_provider/dependency_test.go
- external-providers/java-external-provider/pkg/java_external_provider/provider_test.go
✅ Files skipped from review due to trivial changes (1)
- external-providers/generic-external-provider/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (10)
- external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/App.java
- external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go
- external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go
- external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go
- rule-example.yaml
- Makefile
- external-providers/java-external-provider/pkg/java_external_provider/dependency/constants.go
- external-providers/java-external-provider/pkg/java_external_provider/provider.go
- external-providers/java-external-provider/pkg/java_external_provider/bldtool/dep_cache.go
- external-providers/java-external-provider/pkg/java_external_provider/dependency/jar_explode.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-30T12:11:45.673Z
Learnt from: pranavgaikwad
PR: konveyor/analyzer-lsp#859
File: external-providers/java-external-provider/pkg/java_external_provider/dependency.go:694-694
Timestamp: 2025-07-30T12:11:45.673Z
Learning: In the Java external provider dependency walker (external-providers/java-external-provider/pkg/java_external_provider/dependency.go), errors from toDependency function calls should be ignored as they are not considered important by the maintainers.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/dependency/project_create_test.go.gitignoreexternal-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.goexternal-providers/java-external-provider/pkg/java_external_provider/dependency/jar.goexternal-providers/java-external-provider/pkg/java_external_provider/dependency.go
📚 Learning: 2025-08-21T12:39:46.778Z
Learnt from: jmle
PR: konveyor/analyzer-lsp#888
File: external-providers/java-external-provider/pkg/java_external_provider/provider.go:662-663
Timestamp: 2025-08-21T12:39:46.778Z
Learning: In the konveyor/analyzer-lsp Java provider, when setting JAVA_HOME for Gradle commands using exec.CommandContext, the maintainers prefer not to preserve the existing environment variables (including PATH) and use `cmd.Env = append(cmd.Env, fmt.Sprintf("JAVA_HOME=%s", javaHome))` instead of `cmd.Env = append(os.Environ(), fmt.Sprintf("JAVA_HOME=%s", javaHome))`.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
📚 Learning: 2025-08-21T12:42:54.499Z
Learnt from: jmle
PR: konveyor/analyzer-lsp#888
File: external-providers/java-external-provider/pkg/java_external_provider/service_client.go:507-521
Timestamp: 2025-08-21T12:42:54.499Z
Learning: In the konveyor/analyzer-lsp Java external provider (external-providers/java-external-provider/pkg/java_external_provider/service_client.go), the maintainers prefer to keep the current JAVA_HOME selection logic in GetJavaHomeForGradle method simple, as they operate in a controlled environment where both Java 8 and Java 17+ are guaranteed to be available, with Java 17+ being a system requirement for running the analyzer.
Applied to files:
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go
🧬 Code graph analysis (20)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go (2)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (1)
Downloader(38-40)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
MvnURIPrefix(22-22)
external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go (5)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (4)
ResolverOptions(74-121)Resolver(26-70)CopyFile(143-162)AppendToFile(164-185)tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (2)
DecompilerOpts(124-131)DefaultWorkerPoolSize(40-40)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(25-32)
external-providers/java-external-provider/pkg/java_external_provider/dependency/explosion.go (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
DirPermRWXGrp(34-34)
external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go (3)
tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (5)
DirPermRWXGrp(34-34)JavaArchive(18-18)WebArchive(19-19)METAINF(27-27)PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
CopyFile(143-162)
external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go (3)
tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (4)
DirPermRWXGrp(34-34)WEBINF(28-28)METAINF(27-27)PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
CopyFile(143-162)
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompiler_test.go (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(25-32)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
DecompilerOpts(124-131)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
engine/labels/labels.go (2)
AsString(35-40)NewLabelSelector(94-125)
external-providers/java-external-provider/pkg/java_external_provider/dependency/project_create_test.go (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(25-32)
external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go (5)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
ResolverOptions(74-121)Resolver(26-70)tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
DecompilerOpts(124-131)DefaultWorkerPoolSize(40-40)JavaArchive(18-18)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(25-32)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(25-32)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
DirPermRWX(33-33)FilePermRW(35-35)PomXmlFile(23-23)
external-providers/java-external-provider/pkg/java_external_provider/dependency/jar.go (4)
tracing/trace.go (1)
StartNewSpan(65-69)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (2)
ToDependency(57-76)JavaArtifact(25-32)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
EMBEDDED_KONVEYOR_GROUP(39-39)DecomplierResponse(94-98)DirPermRWXGrp(34-34)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
CopyFile(143-162)
external-providers/java-external-provider/pkg/java_external_provider/service_client.go (2)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (1)
BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
CanRestrictSelector(203-213)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact_test.go (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(25-32)
external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
external-providers/golang-dependency-provider/main.go (1)
GetDependenciesDAG(59-90)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/gradle.go (4)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (2)
BuildToolOptions(139-147)BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
Resolver(26-70)ResolverOptions(74-121)external-providers/java-external-provider/pkg/java_external_provider/dependency/gradle_resolver.go (1)
GetGradleResolver(35-47)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go (5)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (2)
BuildToolOptions(139-147)BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
Resolver(26-70)ResolverOptions(74-121)external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go (1)
GetMavenResolver(31-42)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver_test.go (3)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
CopyFile(143-162)ResolverOptions(74-121)external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go (1)
GetBinaryResolver(23-35)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (2)
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (1)
JavaArtifact(25-32)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go (6)
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (2)
Resolver(26-70)ResolverOptions(74-121)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (2)
BuildToolOptions(139-147)BuildTool(45-127)external-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go (1)
Labeler(39-42)external-providers/java-external-provider/pkg/java_external_provider/dependency/binary_resolver.go (1)
GetBinaryResolver(23-35)external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (1)
PomXmlFile(23-23)external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (3)
ToDependency(57-76)JavaArtifact(25-32)ToFilePathDependency(247-257)
🪛 markdownlint-cli2 (0.18.1)
external-providers/java-external-provider/docs/architecture.md
29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
190-190: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
324-324: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
369-369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
444-444: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
503-503: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
532-532: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
577-577: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
613-613: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
696-696: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
821-821: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
871-871: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
894-894: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
961-961: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1012-1012: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1039-1039: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1083-1083: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1193-1193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1235-1235: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1443-1443: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1519-1519: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1573-1573: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1658-1658: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1671-1671: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1684-1684: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1702-1702: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go
Outdated
Show resolved
Hide resolved
...rnal-providers/java-external-provider/pkg/java_external_provider/dependency/labels/labels.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go
Outdated
Show resolved
Hide resolved
shawn-hurley
force-pushed
the
refactor/update-java-dependency
branch
from
0c778a2 to
ac46dd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
external-providers/java-external-provider/pkg/java_external_provider/provider.go (3)
551-568: Unsafe type assertion on dep.Extras[pomPathKey]; also pomPathKey removed from required keys.Guard the lookup to avoid panics; fall back to the provided file when unset.
- path := dep.Extras[pomPathKey].(string) - if path == "" { - path = file - } + path := file + if v, ok := dep.Extras[pomPathKey]; ok { + if s, ok := v.(string); ok && s != "" { + path = s + } + } if path == "" { return location, fmt.Errorf("unable to get location for dep %s, empty pom path", dep.Name) }
637-653: Settings writer bug: Fprint doesn’t format; also permissions too permissive.Use Fprintf to substitute %v. Recommend tightening perms and MkdirAll.
-settingsFilePath := filepath.Join(homeDir, ".analyze", "globalSettings.xml") -err = os.Mkdir(filepath.Join(homeDir, ".analyze"), 0777) +settingsDir := filepath.Join(homeDir, ".analyze") +settingsFilePath := filepath.Join(settingsDir, "globalSettings.xml") +err = os.MkdirAll(settingsDir, 0o700) @@ -err = os.Chmod(settingsFilePath, 0777) +err = os.Chmod(settingsFilePath, 0o600) @@ -_, err = fmt.Fprint(f, fileContentTemplate, m2CacheDir) +_, err = fmt.Fprintf(f, fileContentTemplate, m2CacheDir)
681-689: Java version check uses string comparison; allow Java 8 incorrectly.Parse as integers before comparison.
- re := regexp.MustCompile(`version\s"(\d+)[.\d]*"`) + re := regexp.MustCompile(`version\s"(\d+)[.\d]*"`) matches := re.FindStringSubmatch(string(out)) if len(matches) > 1 { - javaVersion := matches[1] - if majorVersion := javaVersion; majorVersion < "17" { + javaVersion := matches[1] + if mv, err := strconv.Atoi(javaVersion); err == nil { + if mv < 17 { + return "", errors.New("jdtls requires at least Java 17") + } + return javaExecutable, nil + } - return "", errors.New("jdtls requires at least Java 17") - } - return javaExecutable, nil }
♻️ Duplicate comments (43)
external-providers/java-external-provider/docs/architecture.md (2)
321-321: [DUPLICATE] Clarify Gradle and Java version compatibility guidance (line 321).This issue was flagged in a prior review. The documentation states "Java 8 for Gradle ≤8.14, Java 17+ otherwise," which is misleading. Per the Gradle documentation, Gradle 8.x supports Java 8 or later (minimum Java 8). If the implementation intentionally enforces Java 17+ as a policy choice despite broader Gradle compatibility, document that rationale explicitly.
590-590: [DUPLICATE] Fix typos: "exload/expload" → "explode" (multiple locations).These misspellings were flagged in prior reviews and remain unfixed:
- Line 590:
jarExloadArtifact→jarExplodeArtifact- Line 665:
exploadArtifact→explodeArtifact- Lines 1853–1855:
jar_expload.go→jar_explode.goEnsure identifiers and filenames match the actual codebase (verify against the Go source files).
Also applies to: 665-665, 1853-1855
external-providers/java-external-provider/pkg/java_external_provider/bldtool/dep_cache.go (2)
11-17: Critical: Nil mutex panic risk remains.The
hashSync *sync.Mutexpointer can be nil and cause panics. Storesync.Mutexby value instead.This issue was previously flagged. Apply the suggested fix from the past review:
type depCache struct { hashFile string - hash *string // SHA256 hash of pom.xml for caching - hashSync *sync.Mutex // Mutex for thread-safe hash access + hash *string // SHA256 hash of build file for caching + hashSync sync.Mutex // Mutex for thread-safe hash access deps map[uri.URI][]provider.DepDAGItem depLog logr.Logger }Then update call sites to use
d.hashSync.Lock()instead of needing to check for nil.
38-47: Critical: Deadlock on error path and err shadowing.If
getHashfails, the function returns without unlockinghashSync, causing a deadlock. Additionally, the inputerrparameter is shadowed and dropped.This issue was previously flagged. Apply the suggested fix from the past review:
-func (d *depCache) setCachedDeps(deps map[uri.URI][]provider.DepDAGItem, err error) error { - hashString, err := getHash(d.hashFile) - if err != nil { - d.depLog.Error(err, "unable to generate hash from pom file") - return err - } - d.deps = deps - d.hash = &hashString - d.hashSync.Unlock() - return nil +func (d *depCache) setCachedDeps(deps map[uri.URI][]provider.DepDAGItem, setErr error) error { + defer d.hashSync.Unlock() + hashString, hashErr := getHash(d.hashFile) + if hashErr != nil { + d.depLog.Error(hashErr, "unable to generate hash from build file") + return hashErr + } + d.deps = deps + d.hash = &hashString + return setErr }external-providers/java-external-provider/pkg/java_external_provider/dependency/artifact.go (2)
78-78: Critical: Data race on package-level variable.The
mavenSearchErrorCachevariable is accessed concurrently without synchronization, causing a data race.A previous review provided a detailed fix using
sync.Mutex. Please refer to that comment for the complete solution.
283-345: Critical: Index search opens wrong file and uses incorrect algorithm.The
search()function opens the data file but passes it tosearchIndex(), which expects the index file. Additionally,searchIndex()seeks by bytes on a line-based format instead of using the existingreadEntryAt()for fixed-size entries.A previous review provided a comprehensive fix for the search algorithm. Please refer to that detailed comment for the correct implementation.
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (7)
271-346: Extract helper to build baseArtifact/explodeArtifact to remove duplication.Three cases repeat the same baseArtifact construction.
Example helper:
func (d *decompiler) newBaseArtifact(artifactPath string, ch chan DecompilerResponse, wg *sync.WaitGroup) baseArtifact { return baseArtifact{ artifactPath: artifactPath, m2Repo: d.m2Repo, decompileTool: d.decompileTool, javaPath: d.java, labeler: d.labeler, mavenIndexPath: d.mavenIndexPath, decompiler: d, decompilerResponses: ch, decompilerWG: wg, } }Use it in each case to populate explodeArtifact.baseArtifact and only set case-specific fields.
395-464: Apply the same helper in getIntoProjectJobInternal.Duplicate block mirrors getIntoProjectJob.
94-98: Rename DecomplierResponse → DecompilerResponse; fix field typo.Spelling errors in public type/field.
-type DecomplierResponse struct { +type DecompilerResponse struct { Artifacts []JavaArtifact - ouputLocationBase string + outputLocationBase string err error }Also update all references in this file:
- chan DecompilerResponse at lines: 66, 101-103, 166, 220-221, 271, 299, 321, 351, 373, 395.
124-131: Rename labler → labeler across options and constructor.Public API field is misspelled; constructor uses the misspelling.
type DecompilerOpts struct { DecompileTool string log logr.Logger workers int - labler labels.Labeler + labeler labels.Labeler m2Repo string mavenIndexPath string } @@ d := decompiler{ decompileTool: options.DecompileTool, log: log, workers: options.workers, - labeler: options.labler, + labeler: options.labeler, jobs: make(chan decompileJob, 30), java: java, m2Repo: options.m2Repo, mavenIndexPath: options.mavenIndexPath, }Update call sites (tests, providers) to use options.labeler.
Also applies to: 136-145
100-107: Expose Close() to stop workers and avoid leaks.Workers run indefinitely; expose shutdown on the interface and impl.
type Decompiler interface { DecompileIntoProject(context context.Context, binaryPath, projectPath string) ([]JavaArtifact, error) Decompile(context context.Context, binaryPath string) ([]JavaArtifact, error) + Close() error }Add implementation (place in this file):
func (d *decompiler) Close() error { if d.cancelWorkersFunc != nil { d.cancelWorkersFunc() } return nil }Update callers (tests, providers) to defer decompiler.Close().
Also applies to: 111-121
183-216: Remove extra Done() and stray debug log; receiver should only aggregate.Calling job.decompilerWG.Done() here double-decrements the WaitGroup; also “here” log is leftover debug.
-err := job.Run(ctx, d.log) -d.log.Info("here") +err := job.Run(ctx, d.log) @@ - case resp := <-responseChannel: - job.decompilerWG.Done() + case resp := <-responseChannel: if resp.err != nil { errs = append(errs, resp.err) }
462-462: Fix typo in error string.“fo rthe” → “for the”.
- return nil, fmt.Errorf("unable to get a job fo rthe artifact") + return nil, fmt.Errorf("unable to get a job for the artifact")external-providers/java-external-provider/pkg/java_external_provider/dependency.go (1)
31-35: Tighten logging: fix key typo and reduce verbosity/object dump.- p.log.V(4).Info("running dependency analysis for DAG") - p.log.Info("using bldtooL", "tool", fmt.Sprintf("%#v", p.buildTool)) + p.log.V(4).Info("running dependency analysis for DAG") + p.log.V(5).Info("selected build tool", "type", fmt.Sprintf("%T", p.buildTool))external-providers/java-external-provider/pkg/java_external_provider/dependency/decompiler_test.go (3)
450-456: Fix artifactId typo: commons-loging → commons-logging.- ArtifactId: "commons-loging", + ArtifactId: "commons-logging",
556-575: Fix GroupId typos: wasdeb → wasdev (path-form).- GroupId: "net/wasdeb.wlp.sample", + GroupId: "net/wasdev.wlp.sample", @@ - GroupId: "net/wasdeb.wlp.sample", + GroupId: "net/wasdev.wlp.sample", @@ - GroupId: "net/wasdeb.wlp.sample", + GroupId: "net/wasdev.wlp.sample",
654-657: Fix inverted assertion; otherwise mismatches won’t fail tests.- if len(test.artifacts) != len(artifacts) && reflect.DeepEqual(test.artifacts, artifacts) { + if len(test.artifacts) != len(artifacts) || !reflect.DeepEqual(test.artifacts, artifacts) { t.Logf("Artifacts Not Equal:\nexpected: %v\nactual: %v", test.artifacts, artifacts) t.Fail() }external-providers/java-external-provider/pkg/java_external_provider/filter.go (1)
213-215: Return correct uri.URI type on error.Second return value currently a string; breaks build.
- if err != nil { - return "", "", err - } + if err != nil { + return "", uri.URI(""), err + }external-providers/java-external-provider/pkg/java_external_provider/dependency/explosion.go (2)
23-26: Use os.MkdirTemp and handle errors; current tmp naming has low entropy and ignores errors.Replace manual TempDir+rand with MkdirTemp; check and propagate errors; set perms after creation.
- // First we are going to explode the artifact to a tmp directory. - tmpDir := os.TempDir() - tmpDir = filepath.Join(tmpDir, fmt.Sprintf("explode-%s-%v", strings.TrimSuffix(filepath.Base(e.artifactPath), filepath.Ext(e.artifactPath)), rand.IntN(100))) - log.V(7).Info("exploding into tmpDir", "tmpDir", tmpDir) - os.MkdirAll(filepath.Clean(tmpDir), DirPermRWXGrp) + // Create a unique temp directory for explosion. + artifactBase := strings.TrimSuffix(filepath.Base(e.artifactPath), filepath.Ext(e.artifactPath)) + tmpDir, err := os.MkdirTemp("", fmt.Sprintf("explode-%s-", artifactBase)) + if err != nil { + return "", fmt.Errorf("create temp dir: %w", err) + } + log.V(7).Info("exploding into tmpDir", "tmpDir", tmpDir) + _ = os.Chmod(tmpDir, DirPermRWXGrp)
29-35: Harden archive extraction; avoid Zip Slip and handle missing jar tool.Prefer archive/zip with path sanitization; if keeping “jar”, verify availability and still sanitize. Current approach can write outside tmpDir and fails if jar not present.
- cmd := exec.CommandContext(ctx, "jar", "-xvf", e.artifactPath) - cmd.Dir = tmpDir - err := cmd.Run() - if err != nil { - log.V(7).Error(err, "exploding into tmpDir error", "tmpDir", tmpDir) - return "", err - } + if p, lookErr := exec.LookPath("jar"); lookErr == nil { + cmd := exec.CommandContext(ctx, p, "-xvf", e.artifactPath) + cmd.Dir = tmpDir + if err := cmd.Run(); err != nil { + log.V(7).Error(err, "jar extraction failed", "tmpDir", tmpDir) + return "", err + } + } else { + // Fallback: Go-based safe extraction + if err := extractZipToDir(e.artifactPath, tmpDir); err != nil { + log.V(7).Error(err, "zip extraction failed", "tmpDir", tmpDir) + return "", err + } + }Add a safe helper (outside this hunk):
import "archive/zip" func extractZipToDir(zipPath, dest string) error { r, err := zip.OpenReader(zipPath) if err != nil { return err } defer r.Close() for _, f := range r.File { clean := filepath.Clean(f.Name) if strings.HasPrefix(clean, "..") || filepath.IsAbs(f.Name) { return fmt.Errorf("unsafe path in archive: %q", f.Name) } target := filepath.Join(dest, clean) if f.FileInfo().IsDir() { if err := os.MkdirAll(target, DirPermRWXGrp); err != nil { return err } continue } if err := os.MkdirAll(filepath.Dir(target), DirPermRWXGrp); err != nil { return err } rc, err := f.Open() if err != nil { return err } func() { defer rc.Close() out, err := os.Create(target) if err != nil { rc.Close(); panic(err) } defer out.Close() _, err = io.Copy(out, rc) if err != nil { panic(err) } }() } return nil }external-providers/java-external-provider/pkg/java_external_provider/dependency/jar_explode.go (4)
27-35: Fix tracing lifecycle; remove ctx.Done misuse and defer span.End().Defer span.End() right after creation; don’t call jobCtx.Done() (it’s a channel). Also use the derived logger/context consistently.
- j.ctx = ctx - j.log = log.WithName("explode_jar").WithValues("archive", filepath.Base(j.artifactPath)) - jobCtx, span := tracing.StartNewSpan(ctx, "jar-explode-artifact-job") + j.ctx = ctx + j.log = log.WithName("explode_jar").WithValues("archive", filepath.Base(j.artifactPath)) + _, span := tracing.StartNewSpan(ctx, "jar-explode-artifact-job") + defer span.End() @@ - j.tmpDir, err = j.explodeArtifact.ExplodeArtifact(ctx, log) + j.tmpDir, err = j.explodeArtifact.ExplodeArtifact(j.ctx, j.log) @@ - span.End() - jobCtx.Done() + // span is deferredAlso applies to: 47-49
33-35: Initialize foundClassDirs to avoid panic.Writes to a nil map will panic. Initialize before use.
j.tmpDir, err = j.explodeArtifact.ExplodeArtifact(j.ctx, j.log) +j.foundClassDirs = make(map[string]struct{})Also applies to: 106-121
53-57: Honor WalkDir incoming error first.Return err immediately before further processing.
-func (j *jarExplodeArtifact) HandleFile(path string, d fs.DirEntry, err error) error { - absPath, err := filepath.Abs(path) +func (j *jarExplodeArtifact) HandleFile(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + absPath, err := filepath.Abs(path)
84-91: Tighten lib/classes detection and decompile the correct classes root; use request context and configured java.Current strings.Contains checks are brittle; decompile path picks top-level segment (e.g., “WEB-INF”), not the classes root; and it ignores cancellation by using Background and hard-coded “java”.
- if strings.Contains(outputPath, "lib") { + if pathContainsDir(relPath, "lib") { @@ - if strings.Contains(outputPath, "class") { + if pathContainsDir(relPath, "classes") { // get directory from the base of tmp. - rel, err := filepath.Rel(j.tmpDir, absPath) + rel, err := filepath.Rel(j.tmpDir, absPath) if err != nil { return err } - parts := strings.Split(rel, string(filepath.Separator)) - var dirToCreate string - if len(parts) == 0 { - dirToCreate = relPath - } else { - dirToCreate = parts[0] - } - if _, ok := j.foundClassDirs[dirToCreate]; ok { + classesRoot := classesRootDir(rel) // e.g., "WEB-INF/classes" | "BOOT-INF/classes" + if classesRoot == "" { + return nil + } + if _, ok := j.foundClassDirs[classesRoot]; ok { return nil } - err = os.MkdirAll(filepath.Join(j.outputPath, JAVA, dirToCreate), DirPermRWXGrp) + err = os.MkdirAll(filepath.Join(j.outputPath, JAVA, classesRoot), DirPermRWXGrp) if err != nil { j.log.Info("here failed to create dir") return err } - decompileCommand := exec.CommandContext(context.Background(), "java", "-jar", j.decompileTool, filepath.Join(j.tmpDir, dirToCreate), filepath.Join(j.outputPath, JAVA+"/", dirToCreate)) + decompileCommand := exec.CommandContext(j.ctx, j.javaPath, "-jar", j.decompileTool, + filepath.Join(j.tmpDir, classesRoot), + filepath.Join(j.outputPath, JAVA, classesRoot)) err = decompileCommand.Run() if err != nil { j.log.Info("here failed to decompile", "err", err) return err } - j.foundClassDirs[dirToCreate] = struct{}{} + j.foundClassDirs[classesRoot] = struct{}{} return nilAdd helpers (outside this hunk):
func pathContainsDir(relPath, dir string) bool { parts := strings.Split(relPath, string(filepath.Separator)) for _, p := range parts { if p == dir { return true } } return false } func classesRootDir(rel string) string { parts := strings.Split(rel, string(filepath.Separator)) for i, p := range parts { if p == "classes" && i > 0 { return filepath.Join(parts[:i+1]...) } } return "" }Also applies to: 93-121
external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go (2)
76-83: Propagate cancellation to decompiler.Use w.ctx instead of context.Background().
- decompileCommand := exec.CommandContext(context.Background(), w.javaPath, "-jar", w.decompileTool, absPath, outputPath) + decompileCommand := exec.CommandContext(w.ctx, w.javaPath, "-jar", w.decompileTool, absPath, outputPath)
90-97: Create the actual parent directory; current code creates “.”.Use filepath.Dir(outputPath), not Dir(Base(outputPath)).
- if err = os.MkdirAll(filepath.Dir(filepath.Base(outputPath)), DirPermRWXGrp); err != nil { + if err = os.MkdirAll(filepath.Dir(outputPath), DirPermRWXGrp); err != nil { return err }external-providers/java-external-provider/pkg/java_external_provider/dependency/ear.go (2)
109-115: Propagate cancellation to decompiler.Use e.ctx instead of context.Background().
- decompileCommand := exec.CommandContext(context.Background(), e.javaPath, "-jar", e.decompileTool, absPath, outputPath) + decompileCommand := exec.CommandContext(e.ctx, e.javaPath, "-jar", e.decompileTool, absPath, outputPath)
122-127: Create directory itself, not its parent.For directory entries, mkdir outputPath.
- if d.IsDir() { - if err = os.MkdirAll(filepath.Dir(outputPath), DirPermRWXGrp); err != nil { + if d.IsDir() { + if err = os.MkdirAll(outputPath, DirPermRWXGrp); err != nil { return err } return nil }external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven.go (1)
79-94: Don’t unpack binary JARs for .java; prefer sources JAR and verify existence.Binary JARs contain .class. This likely returns non-existent paths and pollutes dirs. Use “-sources.jar” when present; otherwise defer to resolver/decompiler.
func (m *mavenBuildTool) GetSourceFileLocation(packagePath string, jarPath string, javaFileName string) (string, error) { - javaFileAbsolutePath := filepath.Join(filepath.Dir(jarPath), filepath.Dir(packagePath), javaFileName) - - // attempt to decompile when directory for the expected java file doesn't exist - // if directory exists, assume .java file is present within, this avoids decompiling every Jar - if _, err := os.Stat(filepath.Dir(javaFileAbsolutePath)); err != nil { - cmd := exec.Command("jar", "xf", filepath.Base(jarPath)) - cmd.Dir = filepath.Dir(jarPath) - err := cmd.Run() - if err != nil { - m.log.Error(err, "error unpacking java archive") - return "", err - } - } - return javaFileAbsolutePath, nil + baseDir := filepath.Dir(jarPath) + srcJar := jarPath + // Prefer adjacent sources jar: artifact-version-sources.jar + if strings.HasSuffix(strings.ToLower(jarPath), ".jar") && + !strings.Contains(strings.ToLower(filepath.Base(jarPath)), "-sources") { + cand := strings.TrimSuffix(jarPath, ".jar") + "-sources.jar" + if _, err := os.Stat(cand); err == nil { + srcJar = cand + baseDir = filepath.Dir(cand) + } else { + return "", fmt.Errorf("no sources jar found for %q; use resolver/decompiler", jarPath) + } + } + javaFileAbsolutePath := filepath.Join(baseDir, filepath.Dir(packagePath), javaFileName) + // Unpack sources jar on demand + if _, err := os.Stat(filepath.Dir(javaFileAbsolutePath)); err != nil { + cmd := exec.Command("jar", "xf", filepath.Base(srcJar)) + cmd.Dir = baseDir + if runErr := cmd.Run(); runErr != nil { + m.log.Error(runErr, "error unpacking sources archive") + return "", runErr + } + } + if _, err := os.Stat(javaFileAbsolutePath); err != nil { + return "", fmt.Errorf("source file not found after extraction: %s", javaFileAbsolutePath) + } + return javaFileAbsolutePath, nil }external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_downloader.go (1)
64-69: Rely on Maven’s reported destination; remove brittle filename inference.Coordinates→filename mapping here is error-prone (classifier/packaging). Parse “Copying … to …” and error if missing.
- downloadedPath := filepath.Join(outputDir, - fmt.Sprintf("%s.jar", strings.Join(mvnCoordinatesParts[1:3], "-"))) - if len(mvnCoordinatesParts) == 4 { - downloadedPath = filepath.Join(outputDir, - fmt.Sprintf("%s.%s", strings.Join(mvnCoordinatesParts[1:3], "-"), strings.ToLower(mvnCoordinatesParts[3]))) - } + downloadedPath := "" outputLinePattern := regexp.MustCompile(`.*?Copying.*?to (.*)`) for _, line := range strings.Split(string(mvnOutput), "\n") { if outputLinePattern.MatchString(line) { match := outputLinePattern.FindStringSubmatch(line) if match != nil { downloadedPath = match[1] } } } + if downloadedPath == "" { + return "", fmt.Errorf("unable to determine downloaded artifact path from Maven output for coordinates %q", mvnCoordinates) + }Also applies to: 70-79
external-providers/java-external-provider/pkg/java_external_provider/provider.go (1)
509-511: Return the correct error variable (returnErr), noterr.This is the same scope issue previously flagged. Use returnErr.
- if returnErr != nil { - return nil, additionalBuiltinConfig, err - } + if returnErr != nil { + return nil, additionalBuiltinConfig, returnErr + }external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go (1)
84-102: Preserve unresolved property tokens when key missing.Set dep.Version to the original ${key} when property is absent; current code sets bare key.
- } else { - version = pom.Properties.Entries[version] - if version != "" { - dep.Version = version - } + } else { + resolved := pom.Properties.Entries[version] + if resolved != "" { + dep.Version = resolved + } else { + dep.Version = fmt.Sprintf("${%s}", version) + }external-providers/java-external-provider/pkg/java_external_provider/service_client.go (1)
121-123: Guard p.buildTool before GetLocalRepoPath().Avoid nil deref when project bootstraps without a build tool.
- "mavenLocalRepo": p.buildTool.GetLocalRepoPath(), + "mavenLocalRepo": func() string { + if p.buildTool != nil { + return p.buildTool.GetLocalRepoPath() + } + return "" + }(),external-providers/java-external-provider/pkg/java_external_provider/dependency/maven_resolver.go (1)
87-128: Fix deadlock, race, and wrong JAR path in parallel decompilation.Use a buffered results channel, capture loop var, include version directory in jar path, wait before cancel, and log resp.err.
- wg := &sync.WaitGroup{} - dependencies := []JavaArtifact{} - returnChan := make(chan struct { - artifact []JavaArtifact - err error - }) - decompilerCtx, cancelFunc := context.WithCancel(ctx) + wg := &sync.WaitGroup{} + dependencies := []JavaArtifact{} + results := make(chan struct { + artifact []JavaArtifact + err error + }, len(artifacts)) + decompilerCtx, cancelFunc := context.WithCancel(ctx) @@ - go func() { - for { - select { - case resp := <-returnChan: - defer wg.Done() - if resp.err != nil { - m.log.Error(err, "unable to get java artifact") - continue - } - dependencies = append(dependencies, resp.artifact...) - case <-decompilerCtx.Done(): - return - } - } - }() - for _, artifact := range artifacts { - m.log.WithValues("artifact", artifact).Info("sources for artifact not found, decompiling...") - - groupDirs := filepath.Join(strings.Split(artifact.GroupId, ".")...) - jarName := fmt.Sprintf("%s-%s.jar", artifact.ArtifactId, artifact.Version) - wg.Add(1) - go func() { - artifact, err := decompiler.Decompile(decompilerCtx, filepath.Join(m.localRepo, groupDirs, artifact.ArtifactId, jarName)) - returnChan <- struct { - artifact []JavaArtifact - err error - }{artifact: artifact, err: err} - }() - } - wg.Done() - cancelFunc() + for _, a := range artifacts { + a := a // capture + m.log.WithValues("artifact", a).Info("sources for artifact not found, decompiling...") + groupDirs := filepath.Join(strings.Split(a.GroupId, ".")...) + jarName := fmt.Sprintf("%s-%s.jar", a.ArtifactId, a.Version) + jarPath := filepath.Join(m.localRepo, groupDirs, a.ArtifactId, a.Version, jarName) + wg.Add(1) + go func(j string) { + defer wg.Done() + arts, err := decompiler.Decompile(decompilerCtx, j) + results <- struct { + artifact []JavaArtifact + err error + }{artifact: arts, err: err} + }(jarPath) + } + go func() { + wg.Wait() + close(results) + }() + for resp := range results { + if resp.err != nil { + m.log.Error(resp.err, "unable to get java artifact") + continue + } + dependencies = append(dependencies, resp.artifact...) + } + cancelFunc()Also applies to: 101-103, 113-118
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go (4)
189-196: Don’t recurse WalkDir inside its callback.Let WalkDir handle traversal; current call duplicates recursion and can loop paths.
if info.IsDir() { - return filepath.WalkDir(filepath.Join(path, info.Name()), w.walkDirForJar) + return nil }
259-265: Same recursion issue for POM walker.Remove nested WalkDir call.
if info.IsDir() { - return filepath.WalkDir(filepath.Join(path, info.Name()), w.walkDirForPom) + return nil }
205-217: Handle ToDependency error; also use the correct map key and file URI.Capture/log the error, and store deps under the actual file path (path is already the file).
- artifact, _ := dependency.ToDependency(context.TODO(), w.log, w.labeler, path, w.mavenIndexPath) + artifact, err := dependency.ToDependency(context.TODO(), w.log, w.labeler, path, w.mavenIndexPath) + if err != nil { + w.log.V(5).Error(err, "failed to construct artifact from JAR", "path", path) + } @@ - w.deps[uri.URI(filepath.Join(path, info.Name()))] = []provider.DepDAGItem{ + w.deps[uri.File(path)] = []provider.DepDAGItem{
238-248: Handle ToFilePathDependency error and use file URI for map key.Log the error and prefer file:// URIs for consumers that expect URIs.
- artifact, _ := dependency.ToFilePathDependency(context.Background(), filepath.Join(relPath, info.Name())) + artifact, err := dependency.ToFilePathDependency(context.Background(), filepath.Join(relPath, info.Name())) + if err != nil { + w.log.V(5).Error(err, "failed to construct artifact from file path", "path", relPath) + } @@ - w.deps[uri.URI(filepath.Join(relPath))] = []provider.DepDAGItem{ + w.deps[uri.File(relPath)] = []provider.DepDAGItem{Also applies to: 249-255
external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (4)
164-185: AppendToFile fails if destination doesn't exist; add O_CREATE flag.The function opens the destination file without
os.O_CREATE, causing it to fail when the file doesn't exist.Apply this diff:
- destFile, err := os.OpenFile(dst, os.O_APPEND|os.O_WRONLY, FilePermRW) + destFile, err := os.OpenFile(dst, os.O_APPEND|os.O_WRONLY|os.O_CREATE, FilePermRW)
237-246: Critical logic error: break exits loop prematurely, should continue.When a matching dependency is found, the code uses
breakwhich exits the entire loop, preventing remaining dependencies from being processed. This means only the first dependency in the list is checked.Apply this diff:
for _, artifact := range dependencies { var found bool if slices.ContainsFunc(*pom.Dependencies, artifact.EqualsPomDep) { found = true } if found { - break + continue } foundUpdates = true *pom.Dependencies = append(*pom.Dependencies, artifact.ToPomDep()) }
249-249: Use FilePermRW (0644) instead of DirPermRWX (0755) for file permissions.Files should use
FilePermRW(0644) for proper access control.DirPermRWX(0755) is meant for directories.Apply this diff:
- pomFile, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_TRUNC|os.O_CREATE|os.O_WRONLY, DirPermRWX) + pomFile, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_TRUNC|os.O_CREATE|os.O_WRONLY, FilePermRW)
266-266: Use FilePermRW (0644) instead of DirPermRWX (0755) for file permissions.This file creation should use
FilePermRW(0644) for files, notDirPermRWX(0755) which is for directories.Apply this diff:
- pom, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_CREATE|os.O_WRONLY, DirPermRWX) + pom, err := os.OpenFile(filepath.Join(dir, PomXmlFile), os.O_CREATE|os.O_WRONLY, FilePermRW)external-providers/java-external-provider/pkg/java_external_provider/bldtool/tool.go (1)
169-185: Return hex-encoded hash and defer file close immediately.Two issues:
- The function returns raw bytes converted to string (
string(hash.Sum(nil))), which is non-portable and unreadable. Hash values should be hex-encoded for use as cache keys.- The file is closed manually instead of using
defer, which could leak the file descriptor if a panic occurs between open and close.Apply this diff:
func getHash(path string) (string, error) { hash := sha256.New() - var file *os.File file, err := os.Open(path) if err != nil { if errors.Is(err, os.ErrNotExist) { return "", nil } return "", fmt.Errorf("unable to open the pom file %s - %w", path, err) } + defer file.Close() + if _, err = io.Copy(hash, file); err != nil { - file.Close() return "", fmt.Errorf("unable to copy file to hash %s - %w", path, err) } - file.Close() - return string(hash.Sum(nil)), nil + return fmt.Sprintf("%x", hash.Sum(nil)), nil }
🧹 Nitpick comments (9)
.github/workflows/pr-testing.yml (1)
24-27: Excellent addition to test the Java External Provider.The new workflow step appropriately exercises the substantial Java build-tool framework and dependency resolution changes introduced in this PR.
For better alignment with GitHub Actions best practices and improved portability across the matrix platforms (especially Windows), consider using the
working-directorycontext instead of inlinecd:- name: Test Java External Provider - run: | - cd external-providers/java-external-provider/ - go test -v ./... + working-directory: external-providers/java-external-provider + run: go test -v ./...This approach is clearer, more idiomatic for GitHub Actions, and avoids potential shell-specific path handling issues across platforms.
external-providers/java-external-provider/examples/inclusion-tests/src/main/java/io/konveyor/App.java (1)
15-17: Verify the logic: unused variable and misleading method.Two observations about this code segment:
- The
filevariable is created but never used, making this code effectively a no-op.- Based on the
FileReader.fileExists()implementation (which always returnstruewithout actually checking file existence), the conditional check doesn't add meaningful behavior.Since this is in the
inclusion-testsexamples directory, this may be intentional test code for verifying import/dependency resolution. However, if the goal is to demonstrate actual file existence checking, the implementation would need adjustment.engine/engine.go (1)
572-581: Improved error handling and logging clarity.The changes properly distinguish between actual errors and cases where no code snippet is available:
- Errors are logged at the appropriate level (V(6))
- Empty results without errors are logged informationally (V(3))
- CodeSnip is only assigned when content is available
However, note that the counter at line 580 increments for all attempts (including failures). This means if code snippet retrieval consistently fails for a file, the limit will be exhausted without obtaining any snippets. Consider whether the counter should only increment on successful assignments (within the
elseblock at line 578-579) to ensure the limit applies to successfully retrieved snippets rather than attempts.Apply this diff if you want to count only successful snippet retrievals:
} else if codeSnip == "" { r.logger.V(3).Info("no code snippet returned", "rule", rule) } else { incident.CodeSnip = codeSnip + fileCodeSnipCount[string(m.FileURI)] += 1 } - fileCodeSnipCount[string(m.FileURI)] += 1 }external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go (3)
166-216: Prefer channel-close-after-wait pattern to simplify synchronization.Avoid ctx/cancel + select; close the response channel once producers finish, then range over it. Reduces races and goroutine lifetime.
- responseChannel := make(chan DecomplierResponse) - waitGroup := sync.WaitGroup{} + responseChannel := make(chan DecompilerResponse) + waitGroup := sync.WaitGroup{} @@ - err := job.Run(ctx, d.log) + // Close responseChannel once all jobs have finished. + go func() { + waitGroup.Wait() + close(responseChannel) + }() + err := job.Run(ctx, d.log) @@ - receiverCtx, cancelFunc := context.WithCancel(ctx) - go func() { - for { - select { - case resp := <-responseChannel: - if resp.err != nil { - errs = append(errs, resp.err) - } - artifacts = append(artifacts, resp.Artifacts...) - case <-receiverCtx.Done(): - return - } - } - }() - - waitGroup.Wait() - cancelFunc() + for resp := range responseChannel { + if resp.err != nil { + errs = append(errs, resp.err) + } + artifacts = append(artifacts, resp.Artifacts...) + }
236-263: Apply the same channel-close-after-wait pattern here.Mirror the Decompile() aggregation fix in DecompileIntoProject().
- receiverCtx, cancelFunc := context.WithCancel(ctx) - go func() { - for { - select { - case resp := <-responseChannel: - // Anything coming back here, should be inside an internal calls - // which should handle there own Done for the working group. - if resp.err != nil { - errs = append(errs, resp.err) - } - artifacts = append(artifacts, resp.Artifacts...) - case <-receiverCtx.Done(): - return - } - } - }() + go func() { + waitGroup.Wait() + close(responseChannel) + }() @@ - waitGroup.Wait() - cancelFunc() + for resp := range responseChannel { + if resp.err != nil { + errs = append(errs, resp.err) + } + artifacts = append(artifacts, resp.Artifacts...) + }
135-143: Fallback to java on PATH if JAVA_HOME unset.Joining with empty JAVA_HOME yields “bin/java”; prefer LookPath fallback.
- java := filepath.Join(os.Getenv("JAVA_HOME"), "bin", "java") + javaHome := os.Getenv("JAVA_HOME") + java := filepath.Join(javaHome, "bin", "java") + if javaHome == "" { + if p, err := exec.LookPath("java"); err == nil { + java = p + } + }external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go (1)
161-163: Trim mvn help:evaluate output to remove trailing newline.Prevents “file://…/repo\n” paths.
- return outb.String() + return strings.TrimSpace(outb.String())external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go (1)
197-205: Dedup key too coarse; base filename collides across versions.Prefer a normalized path (e.g., rel path from initial) or groupId/artifact/version when available.
- seenKey := filepath.Base(info.Name()) + rel, _ := filepath.Rel(w.initialPath, path) + seenKey := filepath.Clean(rel)external-providers/java-external-provider/pkg/java_external_provider/dependency/resolver.go (1)
120-120: Add documentation comment for MavenIndexPath field.All other fields in
ResolverOptionshave documentation comments explaining their purpose. Consider adding a comment for consistency.Apply this diff:
+ // MavenIndexPath is the path to the Maven index used for artifact metadata searches. + // Optional path to a Maven index for resolving artifact information. MavenIndexPath string
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_binary.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go
Show resolved
Hide resolved
...rnal-providers/java-external-provider/pkg/java_external_provider/dependency/artifact_test.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/dependency/war.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Outdated
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Show resolved
Hide resolved
external-providers/java-external-provider/pkg/java_external_provider/provider.go
Show resolved
Hide resolved
shawn-hurley
force-pushed
the
refactor/update-java-dependency
branch
from
ac46dd7 to
e064057
Compare
shawn-hurley
force-pushed
the
refactor/update-java-dependency
branch
3 times, most recently
from
296eb26 to
2f54d9b
Compare
| // - Lock is held through dependency resolution on cache miss | ||
| // - Lock is released by setCachedDeps() after updating cache | ||
| // | ||
| // TODO: Handle cached Dep errors |
| mavenBaseTool | ||
| resolveSync *sync.Mutex | ||
| binaryLocation string // Absolute path to the binary artifact (JAR/WAR/EAR) | ||
| disableMavenSearch bool // Whether to disable Maven repository lookups |
There was a problem hiding this comment.
Is this still relevant after adding the local maven index search?
There was a problem hiding this comment.
probably not, can we remove as a follow-up?
| settingsFile string // Path to Maven settings file (currently unused for binary) | ||
| insecure bool // Allow insecure HTTPS (currently unused for binary) | ||
| location string // Absolute path to the binary artifact file | ||
| cleanBin bool // Whether to clean up temporary binary files (currently unused) |
There was a problem hiding this comment.
Is it possible to add this in for debugging purposes? Maybe something to handle after the ls is stopped..?
| // - labeler: Used to determine if dependency is open source (unused in current implementation) | ||
| // | ||
| // Returns the JavaArtifact with coordinates, or empty artifact with error if all strategies fail. | ||
| func ToDependency(_ context.Context, log logr.Logger, labeler labels.Labeler, jarFile string, mavenIndexPath string) (JavaArtifact, error) { |
There was a problem hiding this comment.
not anymore, but we may in the future again. or we may at some point want to add timeout to this method.
I think that if we want to remove it lets do it as a follow up?
external-providers/java-external-provider/pkg/java_external_provider/bldtool/maven_shared.go
Outdated
Show resolved
Hide resolved
* Tried to encapsulate the steps in interface's that the provider and service client will use * Tried to add encapsulation for the different types of binary explosion to make debugging a specific workflow easier * Made handling of decompilation of class files faster by making decompilation happen asyncrounsly * Tried to make it more clear where items were being exploaded to by creating named helper functions. * Tried to make handling of which build tool to use transparent and tried to make the paths easier to follow depending on the build tool. Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
shawn-hurley
force-pushed
the
refactor/update-java-dependency
branch
from
625dbdc to
c11548d
Compare
Signed-off-by: Shawn Hurley <[email protected]>
Signed-off-by: Shawn Hurley <[email protected]>
|
@pranavgaikwad This is basically passing CI, some rules were changed and the test's were updated but the rules were not seeded in the hub AFAICT. |
Signed-off-by: Shawn Hurley <[email protected]>
pranavgaikwad
left a comment
pranavgaikwad
left a comment
There was a problem hiding this comment.
Huge thanks for taking this on!
3 participants
Summary by CodeRabbit
New Features
Documentation
CI
Bug Fixes
Tests