RTECO-1646 - Enhance Maven command with server details and build info collection - #526
RTECO-1646 - Enhance Maven command with server details and build info collection#526fluxxBot wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMaven FlexPack collection now receives Maven arguments and server details, collects artifacts from multiple module locations, resolves deployment repositories, records repository metadata, and applies SHA-256-based build properties. Native Maven execution also improves wrapper discovery and legacy build metadata handling. ChangesMaven FlexPack build-info flow
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MavenCommand
participant MvnUtils
participant FlexPackCollector
participant Artifactory
MavenCommand->>MvnUtils: Execute Maven goals
MvnUtils->>FlexPackCollector: Pass goals and server details
FlexPackCollector->>FlexPackCollector: Collect artifacts from module locations
FlexPackCollector->>Artifactory: Resolve repositories and query SHA-256 matches
FlexPackCollector->>Artifactory: Apply build properties and save build info
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… collection - Added server details to the Maven command execution to improve build info collection. - Introduced new utility functions for extracting resolution arguments and sanitizing packaging types. - Implemented tests for various Maven functionalities including artifact collection and module ID splitting. - Updated the build-info-go dependency to a newer version for enhanced features.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
artifactory/commands/flexpack/maven.go (1)
351-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale doc comment.
Lines 351-352 document
resolutionArgReferencesSettings, which no longer exists. Keep only theisSeparateValueFlagcomment.♻️ Proposed change
-// resolutionArgReferencesSettings reports whether arg selects a settings file (its value may be the -// following token). // isSeparateValueFlag reports whether arg is a resolution flag that consumes the next token as its value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/flexpack/maven.go` around lines 351 - 358, Remove the stale resolutionArgReferencesSettings comment above isSeparateValueFlag, leaving only the documentation that describes isSeparateValueFlag and its separate-value behavior.artifactory/commands/flexpack/maven_test.go (1)
64-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for the space-separated
-Pform.
mvn deploy -P prodis valid Maven. No case covers it, which is why the dangling--Pdefect flagged inartifactory/commands/flexpack/maven.go(lines 370-393) is not detected.💚 Proposed test case
{name: "activate-profiles long form", args: []string{"deploy", "--activate-profiles", "prod,ci"}, want: []string{"--activate-profiles", "prod,ci"}}, + {name: "profiles short flag with separate value", args: []string{"deploy", "-P", "prod,ci"}, want: []string{"-P", "prod,ci"}},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@artifactory/commands/flexpack/maven_test.go` around lines 64 - 92, Add a table-driven test case in TestExtractResolutionArgs for the space-separated Maven profile form, using arguments such as "-P", "prod" and expecting both tokens preserved in order. Ensure the test exercises extractResolutionArgs and detects the dangling "-P" behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@artifactory/commands/flexpack/maven.go`:
- Around line 370-393: Update extractResolutionArgs so exact -P and -D tokens
are handled in the separate-value branch, consuming and forwarding their
following value token. Keep prefixed forms such as -Pprofile and -Dkey=value in
the existing inline-value branch, while preserving the current handling of the
other separate-value flags.
- Around line 217-232: Update tagArtifactsInRepo to inspect reader.GetError()
after SetProps consumes the search reader, before returning the count. Propagate
the reader error when present so streaming or parse failures are returned
instead of reporting zero artifacts; preserve the existing SetProps error
handling and reader.Close behavior.
- Around line 431-455: Update the main artifact resolution around
sanitizePackaging and mainArtifactName so Maven packagings maven-plugin, bundle,
ejb, and maven-archetype use a .jar extension while preserving packagingType as
the artifact Type. If the computed default filename is absent, scan targetDir
for the expected artifact extension to support custom finalName values, then
apply the existing validation and path-safety checks before appending the
artifact.
---
Nitpick comments:
In `@artifactory/commands/flexpack/maven_test.go`:
- Around line 64-92: Add a table-driven test case in TestExtractResolutionArgs
for the space-separated Maven profile form, using arguments such as "-P", "prod"
and expecting both tokens preserved in order. Ensure the test exercises
extractResolutionArgs and detects the dangling "-P" behavior.
In `@artifactory/commands/flexpack/maven.go`:
- Around line 351-358: Remove the stale resolutionArgReferencesSettings comment
above isSeparateValueFlag, leaving only the documentation that describes
isSeparateValueFlag and its separate-value behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56895234-2af5-49b5-aaf5-fd56a561e5d6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
artifactory/commands/flexpack/maven.goartifactory/commands/flexpack/maven_test.goartifactory/commands/mvn/mvn.goartifactory/commands/mvn/utils.gogo.mod
| func tagArtifactsInRepo(servicesManager artifactory.ArtifactoryServicesManager, repo string, sha256s []string, buildProps string) (int, error) { | ||
| reader, err := servicesManager.SearchFiles(services.SearchParams{ | ||
| CommonParams: &specutils.CommonParams{Aql: specutils.Aql{ItemsFind: checksumAql(repo, sha256s)}}, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to resolve absolute path for settings.xml: %w", err) | ||
| return 0, err | ||
| } | ||
| cleanedPath := filepath.Clean(absPath) | ||
| if cleanedPath != absPath { | ||
| return nil, fmt.Errorf("invalid path detected: %s", settingsPath) | ||
| count, setErr := servicesManager.SetProps(services.PropsParams{Reader: reader, Props: buildProps}) | ||
| if closeErr := reader.Close(); closeErr != nil { | ||
| log.Debug("Failed to close search reader: " + closeErr.Error()) | ||
| } | ||
|
|
||
| data, err := os.ReadFile(absPath) | ||
| if err != nil { | ||
| return nil, err | ||
| if setErr != nil { | ||
| return 0, setErr | ||
| } | ||
|
|
||
| var settings SettingsXml | ||
| if err := xml.Unmarshal(data, &settings); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &settings, nil | ||
| return count, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the ContentReader idiom used elsewhere in this repository after SearchFiles/SetProps.
rg -nP -C4 '\bSearchFiles\s*\(' --type=go | head -100
rg -nP -C2 '\bGetError\s*\(\s*\)' --type=go | head -60Repository: jfrog/jfrog-cli-artifactory
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Go toolchain availability:"
command -v go || true
go version 2>/dev/null || true
echo
echo "Locate maven.go and inspect relevant section(s):"
fd -a 'maven\.go$' . | sed 's#^\./##'
file="$(fd 'maven\.go$' . | head -1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '180,250p' "$file" | nl -ba -v180
fi
echo
echo "Search for SearchFiles, SetProps, ContentReader GetError patterns in checked-in Go files:"
rg -n -C4 '\bSearchFiles\s*\(' --ext go . || true
rg -n -C3 '\.GetError\s*\(\s*\)' --ext go . || true
rg -n -C3 '\bSetProps\s*\(' --ext go . || trueRepository: jfrog/jfrog-cli-artifactory
Length of output: 439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository go module info:"
if [ -f go.mod ]; then
rg -n 'jfrog-client-go|jfrog-cli-core|jfrog-jfrog-client-go' go.mod || true
fi
echo
echo "Dependabot/modules references to client-go version/tag in repo files:"
rg -n 'jfrog/jfrog-client-go|jfrog-client-go' -g 'go.mod' -g 'go.sum' . || trueRepository: jfrog/jfrog-cli-artifactory
Length of output: 896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='./artifactory/commands/flexpack/maven.go'
echo "Relevant section of $file:"
awk 'NR>=180 && NR<=250 {printf "%d\t%s\n", NR, $0}' "$file"
echo
echo "SearchFiles/SetProps/GetError usages in $file:"
rg -n -C3 '\bSearchFiles\s*\(|\bSetProps\s*\(|\.GetError\s*\(\s*\)' "$file" || true
echo
echo "Call sites of tagArtifactsInRepo:"
rg -n -C4 'tagArtifactsInRepo\s*\(' "$file" || trueRepository: jfrog/jfrog-cli-artifactory
Length of output: 5725
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Fetch jfrog-client-go ContentReader and SetProps definitions from declared module without installing:"
tmp="$(mktemp -d)"
cd "$tmp"
go env -w GOPRIVATE='' >/dev/null 2>&1 || true
go env >/dev/null 2>&1 || true
cat > go.mod <<'EOF'
module testprobe
go 1.23
require github.com/jfrog/jfrog-client-go v1.55.1-0.20260508101905-a17af78a38d7
EOF
go mod download github.com/jfrog/jfrog-client-go@v1.55.1-0.20260508101905-a17af78a38d7 >/dev/null
echo "Module path:"
go mod edit -json | jq '.Module.Path'
echo
echo "Locate ContentReader and SetProps definitions:"
rg -l 'type ContentReader|func .*SetProps' "$GOPATH/pkg/mod/github.com/jfrog/jfrog-client-go" >/tmp/client_go_matches.txt || true
cat /tmp/client_go_matches.txt
echo
echo "GetError definition usages:"
rg -n -C4 'func .*GetError|type ContentReader|GetError\(\s*\)' "$GOPATH/pkg/mod/github.com/jfrog/jfrog-client-go" >/tmp/content_reader_geterror.txt || true
cat /tmp/content_reader_geterror.txt
echo
echo "SetProps definition(s):"
rg -n -C8 'func .*SetProps|SetPropsParams' "$GOPATH/pkg/mod/github.com/jfrog/jfrog-client-go" >/tmp/setprops.txt || true
cat /tmp/setprops.txtRepository: jfrog/jfrog-cli-artifactory
Length of output: 961
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Fetch jfrog-client-go v1.55.1 source to tmp without touching GOPATH:"
tmp="$(mktemp -d)"
cd "$tmp"
cat > go.mod <<EOF
module testprobe
go 1.23
require github.com/jfrog/jfrog-client-go v1.55.1-0.20260508101905-a17af78a38d7
EOF
env GOPATH="$tmp/gopath" go mod download -json github.com/jfrog/jfrog-client-go@v1.55.1-0.20260508101905-a17af78a38d7
path="$(env GOPATH="$tmp/gopath" go env GOPATH)/pkg/mod/github.com/jfrog/jfrog-client-go@v1.55.1-0.20260508101905-a17af78a38d7"
echo "Module path: $path"
echo
echo "Candidate files:"
find "$path" -type f \( -name 'content_reader.go' -o -name '*setprops*' -o -name '*search*' \) -print | sed "s|$path||" | sort
echo
echo "GetError definitions/usages:"
rg -n -C4 'func .*GetError|type ContentReader|GetError\(\s*\)' "$path" || true
echo
echo "ContentReader type and methods:"
rg -n -C6 'type ContentReader|func \(.*ContentReader\).*GetError|func \(.*ContentReader\).*Close|func \(.*ContentReader\).*ReadFile' "$path" || true
echo
echo "SetProps implementations:"
rg -n -C10 'func .*SetProps|func .*PropsParams|SetPropsParams' "$path" || true
echo
echo "Relevant code snippets extracted by file:"
for f in \
"$path/artifactory/servic"*".go" \
"$path/artifactory/servics/props.go" \
"$path/artifactory/servics/props.go" \
"$path/artifactory/servics/props.go" \
"$path/content/**/*.go" \
"$path/content/**/..." ""
do
[ -f "$f" ] || continue
echo "--- $f"
rg -n -C6 'type ContentReader|func .*GetError|func .*SetProps|reader\.GetError|GetError\(\s*\)' "$f" || true
doneRepository: jfrog/jfrog-cli-artifactory
Length of output: 1377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "GitHub raw source snippets for jfrog-client-go ContentReader and props SetProps:"
base="https://raw.githubusercontent.com/jfrog/jfrog-client-go/v1.55.1-0.20260508101905-a17af105-a17af78a38d7"
printf '\n--- ContentReader definition/usages ---\n'
for f in \
"$base/artifactory/services/props.go" \
"$base/artifactory/services/props.go"; do
[ "${f#*://}" != "$f" ] || continue
echo "URL: $f"
curl -fsSL "$f" 2>/dev/null | rg -n -C8 'type ContentReader|func .*GetError|reader\.GetError|GetError\(\s*\)|func .*SetProps|PropsParams' || true
done
printf '\n--- direct content search ---\n'
curl -fsSL "https://api.github.com/search/code?q=repo:%22jfrog/jfrog-client-go%22+get+ContentReader+GetError+version:%22v1.55.1-0.20260508101905-a17af78a38d7%22" \
| jq -r '.items[].html_url?:' | sort -u | head -20
printf '\n--- props.go from GitHub tag with curl only ---\n'
url="https://raw.githubusercontent.com/jfrog/jfrog-client-go/v1.55.1-0.20260508101905-a17af78a38d7/artifactory/services/props.go"
curl -fsSL "$url" 2>/dev/null | sed -n '1,260p'Repository: jfrog/jfrog-cli-artifactory
Length of output: 809
🌐 Web query:
jfrog-client-go v1.55.1 ContentReader GetError SetProps reader.GetError
💡 Result:
In jfrog-client-go, the ContentReader struct is a utility used to handle large API results (such as search results) efficiently without loading all data into memory [1][2]. The methods you mentioned are used as follows: 1. ContentReader.GetError: This method is called after iterating through the reader's records to check if any errors occurred during the streaming process (e.g., during NextRecord) [1][2]. It should be called after your loop finishes [1]. 2. SetProps: This is a method on the Artifactory services manager (e.g., ArtifactoryServicesManagerImp) used to apply properties to files [3][4]. When using the version of SetProps that operates on a collection of artifacts, it requires a PropsParams struct, which contains a Reader field populated with a ContentReader [3][5]. Typical workflow: 1. Obtain a ContentReader by calling an API like SearchFiles [1][2]. 2. Iterate through the records using reader.NextRecord(target) [1][2]. 3. If necessary, use the reader to set properties via SetProps by assigning the reader to the Reader field in the services.PropsParams struct [5]. 4. Always close the reader using reader.Close to release resources, often deferred immediately after opening [1][2]. 5. Check for any errors encountered during streaming by calling reader.GetError after the iteration [1][2]. Note that there are known concurrency considerations with ContentReader, and recent updates (such as those in 2026) have addressed race conditions involving the internal channels of the reader [6].
Citations:
- 1: https://docs.jfrog.com/integrations/docs/jfrog-go-client
- 2: https://github.com/jfrog/documentation/blob/main/jfrog-applications/ci-and-sdks/sdks/jfrog-go-client.md
- 3: https://pkg.go.dev/github.com/jfrog/jfrog-client-go/artifactory
- 4: https://github.com/jfrog/jfrog-client-go/blob/master/artifactory/manager.go
- 5: https://github.com/jfrog/jfrog-cli-artifactory/blob/86017c7d/artifactory/commands/buildinfo/civcs_props.go
- 6: Fix race in ContentReader causing 'send on closed channel' panic jfrog-client-go#1347
Check the search reader error after SetProps.
SearchFiles returns a ContentReader that reports streaming/parse failures through GetError(). SetProps consumes the reader, so a read failure can leave count at 0 without returning tagErr, and the caller reports “No deployed artifacts found to tag” instead of the search error.
♻️ Proposed change
count, setErr := servicesManager.SetProps(services.PropsParams{Reader: reader, Props: buildProps})
+ if readerErr := reader.GetError(); readerErr != nil {
+ log.Debug("Search reader reported an error: " + readerErr.Error())
+ }
if closeErr := reader.Close(); closeErr != nil {
log.Debug("Failed to close search reader: " + closeErr.Error())
}🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 225-225: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("Failed to close search reader: " + closeErr.Error())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/flexpack/maven.go` around lines 217 - 232, Update
tagArtifactsInRepo to inspect reader.GetError() after SetProps consumes the
search reader, before returning the count. Propagate the reader error when
present so streaming or parse failures are returned instead of reporting zero
artifacts; preserve the existing SetProps error handling and reader.Close
behavior.
| func extractResolutionArgs(userArgs []string) []string { | ||
| var extracted []string | ||
| for i := 0; i < len(userArgs); i++ { | ||
| arg := userArgs[i] | ||
| switch { | ||
| case strings.HasPrefix(arg, "-P"), strings.HasPrefix(arg, "--activate-profiles="), | ||
| strings.HasPrefix(arg, "-D"), strings.HasPrefix(arg, "--define="), | ||
| strings.HasPrefix(arg, "--settings="), strings.HasPrefix(arg, "-f="), | ||
| strings.HasPrefix(arg, "--file="), strings.HasPrefix(arg, "-gs="), | ||
| strings.HasPrefix(arg, "--global-settings="), | ||
| arg == "-o", arg == "--offline": | ||
| extracted = append(extracted, arg) | ||
| case isSeparateValueFlag(arg), | ||
| arg == "--activate-profiles", arg == "--define": | ||
| extracted = append(extracted, arg) | ||
| // These flags take their value as the next token. | ||
| if i+1 < len(userArgs) { | ||
| i++ | ||
| extracted = append(extracted, userArgs[i]) | ||
| } | ||
| } | ||
| } | ||
| return extracted | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle -P with a space-separated value.
Maven accepts mvn deploy -P prod. The first case matches -P through strings.HasPrefix, so the value token prod is dropped and a bare -P is forwarded to the internal mvn dependency:tree call. Maven then either fails with a missing-argument error or consumes the next internal argument. Dependency resolution no longer matches the build, which is the purpose of this function.
Match the exact -P (and -D) tokens in the separate-value branch before the prefix cases.
🐛 Proposed fix
func isSeparateValueFlag(arg string) bool {
- return arg == "-s" || arg == "--settings" ||
+ return arg == "-P" || arg == "-D" ||
+ arg == "-s" || arg == "--settings" ||
arg == "-f" || arg == "--file" ||
arg == "-gs" || arg == "--global-settings"
} switch {
- case strings.HasPrefix(arg, "-P"), strings.HasPrefix(arg, "--activate-profiles="),
+ case isSeparateValueFlag(arg),
+ arg == "--activate-profiles", arg == "--define":
+ extracted = append(extracted, arg)
+ // These flags take their value as the next token.
+ if i+1 < len(userArgs) {
+ i++
+ extracted = append(extracted, userArgs[i])
+ }
+ case strings.HasPrefix(arg, "-P"), strings.HasPrefix(arg, "--activate-profiles="),
strings.HasPrefix(arg, "-D"), strings.HasPrefix(arg, "--define="),
strings.HasPrefix(arg, "--settings="), strings.HasPrefix(arg, "-f="),
strings.HasPrefix(arg, "--file="), strings.HasPrefix(arg, "-gs="),
strings.HasPrefix(arg, "--global-settings="),
arg == "-o", arg == "--offline":
extracted = append(extracted, arg)
- case isSeparateValueFlag(arg),
- arg == "--activate-profiles", arg == "--define":
- extracted = append(extracted, arg)
- // These flags take their value as the next token.
- if i+1 < len(userArgs) {
- i++
- extracted = append(extracted, userArgs[i])
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func extractResolutionArgs(userArgs []string) []string { | |
| var extracted []string | |
| for i := 0; i < len(userArgs); i++ { | |
| arg := userArgs[i] | |
| switch { | |
| case strings.HasPrefix(arg, "-P"), strings.HasPrefix(arg, "--activate-profiles="), | |
| strings.HasPrefix(arg, "-D"), strings.HasPrefix(arg, "--define="), | |
| strings.HasPrefix(arg, "--settings="), strings.HasPrefix(arg, "-f="), | |
| strings.HasPrefix(arg, "--file="), strings.HasPrefix(arg, "-gs="), | |
| strings.HasPrefix(arg, "--global-settings="), | |
| arg == "-o", arg == "--offline": | |
| extracted = append(extracted, arg) | |
| case isSeparateValueFlag(arg), | |
| arg == "--activate-profiles", arg == "--define": | |
| extracted = append(extracted, arg) | |
| // These flags take their value as the next token. | |
| if i+1 < len(userArgs) { | |
| i++ | |
| extracted = append(extracted, userArgs[i]) | |
| } | |
| } | |
| } | |
| return extracted | |
| } | |
| func extractResolutionArgs(userArgs []string) []string { | |
| var extracted []string | |
| for i := 0; i < len(userArgs); i++ { | |
| arg := userArgs[i] | |
| switch { | |
| case isSeparateValueFlag(arg), | |
| arg == "--activate-profiles", arg == "--define": | |
| extracted = append(extracted, arg) | |
| // These flags take their value as the next token. | |
| if i+1 < len(userArgs) { | |
| i++ | |
| extracted = append(extracted, userArgs[i]) | |
| } | |
| case strings.HasPrefix(arg, "-P"), strings.HasPrefix(arg, "--activate-profiles="), | |
| strings.HasPrefix(arg, "-D"), strings.HasPrefix(arg, "--define="), | |
| strings.HasPrefix(arg, "--settings="), strings.HasPrefix(arg, "-f="), | |
| strings.HasPrefix(arg, "--file="), strings.HasPrefix(arg, "-gs="), | |
| strings.HasPrefix(arg, "--global-settings="), | |
| arg == "-o", arg == "--offline": | |
| extracted = append(extracted, arg) | |
| } | |
| } | |
| return extracted | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/flexpack/maven.go` around lines 370 - 393, Update
extractResolutionArgs so exact -P and -D tokens are handled in the
separate-value branch, consuming and forwarding their following value token.
Keep prefixed forms such as -Pprofile and -Dkey=value in the existing
inline-value branch, while preserving the current handling of the other
separate-value flags.
…ion for repository and SHA256 values
cd71b62 to
0e842d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@artifactory/commands/flexpack/maven.go`:
- Around line 145-165: Validate each module’s deployURL against serverDetails
before using extractRepoKeyFromUrl and repository-resolution logic in the
build-property tagging loop. If the URL targets another Artifactory instance,
resolve it with matching server details when supported; otherwise skip
finalization for that module rather than using servicesManager against the
configured server.
In `@artifactory/commands/mvn/utils.go`:
- Around line 271-276: Replace the direct os.WriteFile call in the build-info
update flow with an atomic same-directory temporary-file write: create the temp
file, apply the existing perm mode, write updated, close it successfully, then
rename it over buildInfoPath. Preserve the best-effort debug logging and ensure
cleanup of the temporary file on any failure before the rename.
- Around line 171-173: Resolve the selected Maven server via ServerDetails()
before constructing MvnUtils, including the native-mode path where mc.init() is
skipped. Pass the resolved server details into MvnUtils so
CollectMavenBuildInfoWithFlexPack receives the configured --server-id rather
than nil or the default server.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57c4d15d-ba4a-4fc0-9974-0d4f59005204
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
artifactory/commands/flexpack/maven.goartifactory/commands/flexpack/maven_test.goartifactory/commands/mvn/mvn.goartifactory/commands/mvn/utils.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- artifactory/commands/mvn/mvn.go
- artifactory/commands/flexpack/maven_test.go
- go.mod
| servicesManager, err := utils.CreateServiceManager(serverDetails, -1, 0, false) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create services manager: %w", err) | ||
| } | ||
|
|
||
| // Get Maven artifact info from pom.xml | ||
| groupId, artifactId, version, err := getMavenArtifactCoordinates(workingDir) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get Maven artifact coordinates: %w", err) | ||
| } | ||
|
|
||
| // Get the repository Maven deployed to from settings.xml or pom.xml | ||
| targetRepo, err := getMavenDeployRepository(workingDir) | ||
| if err != nil { | ||
| log.Warn("Could not determine Maven deploy repository, skipping build properties: " + err.Error()) | ||
| return nil | ||
| } | ||
|
|
||
| // Create search pattern for the specific deployed artifacts in the target repository | ||
| artifactPath := fmt.Sprintf("%s/%s/%s/%s/%s-*", | ||
| targetRepo, | ||
| strings.ReplaceAll(groupId, ".", "/"), artifactId, version, artifactId) | ||
|
|
||
| // Search for deployed artifacts using the specific pattern | ||
| searchParams := services.SearchParams{ | ||
| CommonParams: &specutils.CommonParams{ | ||
| Pattern: artifactPath, | ||
| }, | ||
| } | ||
|
|
||
| searchReader, err := servicesManager.SearchFiles(searchParams) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to search for deployed artifacts: %w", err) | ||
| } | ||
| defer func() { | ||
| if closeErr := searchReader.Close(); closeErr != nil { | ||
| log.Debug(fmt.Sprintf("Failed to close search reader: %s", closeErr)) | ||
| // Group each module's artifact checksums by the PHYSICAL repo it deployed to (resolving virtual -> | ||
| // default-deployment). Modules may deploy to DIFFERENT repos, so this is per-module, not one repo | ||
| // for the whole reactor. overrideURL (-DaltDeploymentRepository / settings) applies to every module. | ||
| physicalByKey := make(map[string]string) // repoKey -> physical repo (GetRepository cache) | ||
| sha256sByRepo := make(map[string][]string) // physical repo -> checksums to tag | ||
| for i := range buildInfo.Modules { | ||
| module := &buildInfo.Modules[i] | ||
| deployURL := overrideURL | ||
| if deployURL == "" { | ||
| deployURL = moduleDeployURLs[module.Id] | ||
| } | ||
| }() | ||
|
|
||
| // Filter to only artifacts modified in the last 2 minutes (just deployed) | ||
| cutoffTime := time.Now().Add(-2 * time.Minute) | ||
| var recentArtifacts []specutils.ResultItem | ||
|
|
||
| for item := new(specutils.ResultItem); searchReader.NextRecord(item) == nil; item = new(specutils.ResultItem) { | ||
| // Parse the modified time | ||
| modTime, err := time.Parse("2006-01-02T15:04:05.999Z", item.Modified) | ||
| if err != nil { | ||
| log.Debug("Could not parse modified time for " + item.Name + ": " + err.Error()) | ||
| if deployURL == "" { | ||
| log.Debug("No deployment repository for module " + module.Id + "; skipping its build-property tagging") | ||
| continue | ||
| } | ||
|
|
||
| // Only include artifacts modified after cutoff | ||
| if modTime.After(cutoffTime) { | ||
| recentArtifacts = append(recentArtifacts, *item) | ||
| } | ||
| } | ||
|
|
||
| if len(recentArtifacts) == 0 { | ||
| log.Warn("No recently deployed artifacts found") | ||
| return nil | ||
| } | ||
|
|
||
| // Create build properties in the same format as NPM/traditional implementations | ||
| timestamp := strconv.FormatInt(time.Now().UnixNano()/int64(time.Millisecond), 10) // Unix milliseconds like NPM | ||
| buildProps := fmt.Sprintf("build.name=%s;build.number=%s;build.timestamp=%s", buildName, buildNumber, timestamp) | ||
| if projectKey := buildArgs.GetProject(); projectKey != "" { | ||
| buildProps += fmt.Sprintf(";build.project=%s", projectKey) | ||
| } | ||
|
|
||
| buildProps = civcs.MergeWithUserProps(buildProps, workingDir) | ||
|
|
||
| // Set properties on each recent artifact individually | ||
| for _, artifact := range recentArtifacts { | ||
| // ResultItem has Repo, Path, and Name fields already separated | ||
| // Use AQL to find the exact artifact | ||
| aqlPattern := fmt.Sprintf(`{"repo":"%s","path":"%s","name":"%s"}`, | ||
| targetRepo, artifact.Path, artifact.Name) | ||
|
|
||
| searchParams := services.SearchParams{ | ||
| CommonParams: &specutils.CommonParams{ | ||
| Aql: specutils.Aql{ | ||
| ItemsFind: aqlPattern, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| reader, err := servicesManager.SearchFiles(searchParams) | ||
| if err != nil { | ||
| log.Warn(fmt.Sprintf("Failed to search for artifact %s: %s", artifact.Name, err)) | ||
| repoKey, keyErr := extractRepoKeyFromUrl(deployURL) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep repository resolution on the Maven deployment server.
servicesManager targets serverDetails, but deployURL contributes only its repository key. If Maven deploys to another Artifactory instance, this code can search and tag the same repository key on the configured instance.
Validate that each deployment URL belongs to serverDetails. Otherwise, select server details for that deployment URL or skip finalization.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 161-161: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("No deployment repository for module " + module.Id + "; skipping its build-property tagging")
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/flexpack/maven.go` around lines 145 - 165, Validate each
module’s deployURL against serverDetails before using extractRepoKeyFromUrl and
repository-resolution logic in the build-property tagging loop. If the URL
targets another Artifactory instance, resolve it with matching server details
when supported; otherwise skip finalization for that module rather than using
servicesManager against the configured server.
| // Use FlexPack to collect Maven build info. The user's goals/flags are forwarded so the | ||
| // internal dependency resolution matches the profiles/settings the build ran with. | ||
| err = flexpack.CollectMavenBuildInfoWithFlexPack(workingDir, buildName, buildNumber, mu.buildConf, mu.goals, mu.serverDetails) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline artifactory/commands/mvn --items all --match 'MvnCommand|ServerDetails|Run'
rg -n -C6 '\bServerDetails\s*\(|\.ServerDetails\s*\(|SetServerDetails\s*\(' --type=go .Repository: jfrog/jfrog-cli-artifactory
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- mvn.go outline ---\n'
ast-grep outline artifactory/commands/mvn/mvn.go --items all --match 'MvnCommand|ServerDetails|Run|serverId|Set' || true
printf '\n--- serverDetails usages in mvn package ---\n'
rg -n -C5 'serverDetails|ServerDetails|serverID|server-id|--server-id|SetServerDetails' artifactory/commands/mvn --type=go
printf '\n--- relevant utils.go section ---\n'
sed -n '1,230p' artifactory/commands/mvn/utils.go | nl -ba | sed -n '1,230p'Repository: jfrog/jfrog-cli-artifactory
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- mvn.go outline ---'
ast-grep outline artifactory/commands/mvn/mvn.go --items all --match 'MvnCommand|ServerDetails|Run|serverId|Set' || true
printf '%s\n' ''
printf '%s\n' '--- serverDetails usages in mvn package ---'
rg -n -C5 'serverDetails|ServerDetails|serverID|server-id|--server-id|SetServerDetails' artifactory/commands/mvn --type=go
printf '%s\n' ''
printf '%s\n' '--- relevant utils.go section ---'
sed -n '1,230p' artifactory/commands/mvn/utils.go | nl -ba | sed -n '1,230p'Repository: jfrog/jfrog-cli-artifactory
Length of output: 11586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- MvnUtils serverDetails call sites ---'
rg -n -C4 'SetServerDetails\(' --type=go . | sed -n '1,240p'
printf '%s\n' ''
printf '%s\n' '--- command creation and serverDetails assignment ---'
rg -n -C8 'NewMvnCommand|SetServerDetails|serverDetails|serverID|server-id' . --type=go | sed -n '1,260p'Repository: jfrog/jfrog-cli-artifactory
Length of output: 34486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate mvn cli setup ---'
rg -n -C5 'NewMvnCommand|JFrogCLI|rt_mvn|mvn' --type=go . | sed -n '1,320p'
printf '%s\n' ''
printf '%s\n' '--- core/config server resolution helpers ---'
rg -n -C5 'func ResolveServer|ResolveServer|ServerID|serverID|GetSpecificConfig|GetDefaultServerConf|server-id' jfrog-cli-core v2 config --type=go | sed -n '1,360p' || trueRepository: jfrog/jfrog-cli-artifactory
Length of output: 22844
Resolve the Maven server before forwarding it to FlexPack.
In native mode, Run skips mc.init() and passes mc.serverDetails directly to MvnUtils. Since NewMvnCommand() leaves serverDetails nil, the selected --server-id is not included in the FlexPack build-info extraction, and metadata/dependency tagging can use the configured default server instead. Resolve the server through ServerDetails() before creating MvnUtils.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/mvn/utils.go` around lines 171 - 173, Resolve the
selected Maven server via ServerDetails() before constructing MvnUtils,
including the native-mode path where mc.init() is skipped. Pass the resolved
server details into MvnUtils so CollectMavenBuildInfoWithFlexPack receives the
configured --server-id rather than nil or the default server.
| perm := os.FileMode(0644) | ||
| if info, statErr := os.Stat(buildInfoPath); statErr == nil { | ||
| perm = info.Mode().Perm() | ||
| } | ||
| if err = os.WriteFile(buildInfoPath, updated, perm); err != nil { | ||
| log.Debug("Skipping maven build-mode stamp, could not write build info: " + err.Error()) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Write the build-info update atomically.
os.WriteFile truncates the existing build-info file before it writes the new content. A short write, disk-full error, or interruption can destroy valid extractor output even though this helper treats the stamp as best-effort.
Write to a temporary file in the same directory. Apply the original permissions. Rename the completed file over the original only after a successful close.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 275-275: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: log.Debug("Skipping maven build-mode stamp, could not write build info: " + err.Error())
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@artifactory/commands/mvn/utils.go` around lines 271 - 276, Replace the direct
os.WriteFile call in the build-info update flow with an atomic same-directory
temporary-file write: create the temp file, apply the existing perm mode, write
updated, close it successfully, then rename it over buildInfoPath. Preserve the
best-effort debug logging and ensure cleanup of the temporary file on any
failure before the rename.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes