RTECO-945: Add Alpine APK command support (apk add/upload/config) - #496
RTECO-945: Add Alpine APK command support (apk add/upload/config)#496naveenku-jfrog wants to merge 3 commits into
Conversation
375b261 to
55c9834
Compare
43b71dd to
06b9b68
Compare
06b9b68 to
b09d39c
Compare
b09d39c to
9e2008b
Compare
9e2008b to
76d1d13
Compare
76d1d13 to
a73fb96
Compare
a73fb96 to
6deb42b
Compare
24d0882 to
bf18683
Compare
naveenku-jfrog
changed the title
bf18683 to
aca0f35
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Alpine APK setup, command execution, repository isolation, credential handling, package upload, dependency resolution, and Build Info collection with unit coverage and updated JFrog modules. ChangesAlpine APK support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant SetupCommand
participant ApkCommand
participant ApkUploadCommand
participant apk
participant Artifactory
User->>SetupCommand: configure Alpine repository
SetupCommand->>Artifactory: validate repository and retrieve signing key
SetupCommand->>RepositoriesFile: update Alpine repository configuration
User->>ApkCommand: invoke APK operation
ApkCommand->>apk: run filtered command with repository and HTTP_AUTH
apk-->>ApkCommand: return package state and exit status
User->>ApkUploadCommand: upload local APK
ApkUploadCommand->>apk: inspect package metadata and dependencies
ApkUploadCommand->>Artifactory: upload APK 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
artifactory/commands/alpine/apkcommand.go (1)
459-483: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRe-resolve
apkviaapkPathand drop the redundant lookup.
warnIfApkTooOldshells out toapk --versionby name, re-searchingPATHeven thoughRunalready resolvedapkPath(Line 142); passing the resolved path avoids picking up a different binary. Also,parts[0]is printed as a string whileminoris printed as an int — usingfields[1]directly would be clearer.🤖 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/alpine/apkcommand.go` around lines 459 - 483, Update warnIfApkTooOld to accept the resolved apkPath from Run and pass it to exec.Command instead of re-resolving “apk” through PATH. Remove the redundant lookup, and use the parsed version field directly in the warning message rather than mixing parts[0] with the integer minor value.artifactory/commands/setup/setup.go (1)
779-795: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate Alpine-version detection.
detectAlpineVersionhere anddetectSystemAlpineVersioninartifactory/commands/alpine/apkupload.go(Lines 648-660) parse the same file with the same logic, differing only in thevprefix. Consider exporting one helper and normalizing at the call site.🤖 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/setup/setup.go` around lines 779 - 795, Consolidate the duplicated Alpine release parsing by reusing a single exported helper between detectAlpineVersion and detectSystemAlpineVersion. Move the shared file-reading and version-normalization logic into that helper, then apply the required v-prefix difference at each call site while preserving both existing callers’ behavior.artifactory/commands/alpine/apkcommand_test.go (1)
101-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two tests depend on the ambient
JFROG_CLI_ENV_EXCLUDE.
filterSecretEnvVarsreadscoreutils.EnvExcludefirst; if a developer or CI runner has it set, both tests fail or pass vacuously. Pin it witht.Setenv(coreutils.EnvExclude, "*password*;*secret*;*token*;*key*").🤖 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/alpine/apkcommand_test.go` around lines 101 - 123, Make both TestFilterSecretEnvVars_RemovesSecrets and TestFilterSecretEnvVars_PreservesNonSecrets deterministic by setting coreutils.EnvExclude to "*password*;*secret*;*token*;*key*" with t.Setenv at the start of each test, before calling filterSecretEnvVars.
🤖 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/alpine/apkcommand.go`:
- Around line 283-297: Update userExplicitFlags in the environment-credential
handling flow to represent only credentials explicitly supplied via the --user,
--password, or --server-id flags; do not include apkCmd.serverDetails, which is
populated by default server configuration. Preserve the existing behavior of
honoring pre-set HTTP_AUTH when none of those explicit flags were provided.
In `@artifactory/commands/alpine/apkupload.go`:
- Line 206: Update the uploadURL construction in the APK upload flow to
normalize the Artifactory base URL before joining it with target. Trim trailing
slashes from rtURL, then insert exactly one separator before target.
- Around line 277-289: Update the artifact upload flow around getBuildProps,
SearchFiles, and SetProps: propagate the getBuildProps error instead of ignoring
it, and ensure the ContentReader returned by SearchFiles is closed after
SetProps completes, including when SetProps returns an error.
- Around line 436-460: Update resolveRepoFromRepositoriesFile to parse each
repository line with net/url instead of matching the raw string prefix, so
credentials in the URL userinfo do not prevent resolution. Compare the parsed
URL’s host and normalized path against rtURL’s host and Artifactory path, then
extract the repository key from the remaining path while preserving skipping
invalid, empty, and commented lines.
In `@artifactory/commands/setup/setup.go`:
- Around line 942-959: Update the documentation comment for
apkUpdateRepositories to accurately state that it replaces the existing
repositories file with the Artifactory repository entry, rather than claiming
unrelated third-party entries are preserved. Do not change the write behavior or
add an opt-in gate.
- Around line 700-722: The raw HTTP calls must use the configured Artifactory
client instead of http.DefaultClient. In artifactory/commands/setup/setup.go
lines 700-722, update apkValidateRepositoryExists and the sibling keypair calls
to use the service-manager equivalent such as IsRepoExists, removing apkSetAuth;
in artifactory/commands/alpine/apkupload.go lines 239-260, route the upload PUT
through the Artifactory upload service or configured client, preserving
configured timeouts, transport, authentication, and retry behavior.
- Around line 913-936: Update the non-root branch of apkWriteFile to create and
write the credential-bearing file with restrictive permissions from the start,
replacing the sudo tee-then-chmod sequence with a safe sudo mechanism such as
install -m 600 /dev/stdin or an equivalent umask-controlled command. Preserve
the existing content, path handling, error propagation, and root-path behavior;
do not alter the safe exec.Command usage for perm.
---
Nitpick comments:
In `@artifactory/commands/alpine/apkcommand_test.go`:
- Around line 101-123: Make both TestFilterSecretEnvVars_RemovesSecrets and
TestFilterSecretEnvVars_PreservesNonSecrets deterministic by setting
coreutils.EnvExclude to "*password*;*secret*;*token*;*key*" with t.Setenv at the
start of each test, before calling filterSecretEnvVars.
In `@artifactory/commands/alpine/apkcommand.go`:
- Around line 459-483: Update warnIfApkTooOld to accept the resolved apkPath
from Run and pass it to exec.Command instead of re-resolving “apk” through PATH.
Remove the redundant lookup, and use the parsed version field directly in the
warning message rather than mixing parts[0] with the integer minor value.
In `@artifactory/commands/setup/setup.go`:
- Around line 779-795: Consolidate the duplicated Alpine release parsing by
reusing a single exported helper between detectAlpineVersion and
detectSystemAlpineVersion. Move the shared file-reading and
version-normalization logic into that helper, then apply the required v-prefix
difference at each call site while preserving both existing callers’ 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: 3b632789-efca-4012-a13f-b9f20e8cabff
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
artifactory/commands/alpine/apkcommand.goartifactory/commands/alpine/apkcommand_test.goartifactory/commands/alpine/apkupload.goartifactory/commands/alpine/credentials.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.gogo.mod
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
artifactory/commands/alpine/apkcommand_test.go (1)
208-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the signature-hint result.
This test has no observable assertion. It passes if
emitSignatureHintdoes nothing or emits a hint for every input. Capture the log output, or expose a testable result, and assert that only the signature input produces the hint.🤖 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/alpine/apkcommand_test.go` around lines 208 - 211, Update TestEmitSignatureHint_DetectsPatterns to observe emitSignatureHint output, capturing its log or using a testable return value, and assert that the UNTRUSTED signature input produces a hint while the ordinary input produces none.artifactory/commands/alpine/apkcommand.go (2)
565-599: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winHandle file-name collisions in
fileToDep.
fileToDep[fileName] = ikeeps only the last index for a given file name. If two dependencies map to the same.apkname, only one of them receives checksums, and the other stays unenriched without any log. Store a slice of indices instead.The
aqlJSONStringescaping resolves the earlier raw-interpolation concern in this query.♻️ Proposed refactor
- fileToDep := make(map[string]int, len(missing)) + fileToDeps := make(map[string][]int, len(missing)) names := make([]string, 0, len(missing)) for _, i := range missing { fileName := apkFileNameFromID(deps[i].Id) - fileToDep[fileName] = i - names = append(names, fmt.Sprintf(`{"name":%s}`, aqlJSONString(fileName))) + if _, seen := fileToDeps[fileName]; !seen { + names = append(names, fmt.Sprintf(`{"name":%s}`, aqlJSONString(fileName))) + } + fileToDeps[fileName] = append(fileToDeps[fileName], i) } @@ for item := new(specutils.ResultItem); reader.NextRecord(item) == nil; item = new(specutils.ResultItem) { - i, ok := fileToDep[item.Name] + indices, ok := fileToDeps[item.Name] if !ok { continue } - deps[i].Sha1 = item.Actual_Sha1 - deps[i].Sha256 = item.Sha256 - deps[i].Md5 = item.Actual_Md5 - resolved++ + for _, i := range indices { + deps[i].Sha1 = item.Actual_Sha1 + deps[i].Sha256 = item.Sha256 + deps[i].Md5 = item.Actual_Md5 + resolved++ + } }🤖 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/alpine/apkcommand.go` around lines 565 - 599, Update the checksum enrichment flow around fileToDep and the reader.NextRecord loop to retain all dependency indices for each file name rather than overwriting duplicates. Map each item.Name to its slice of indices, then apply Actual_Sha1, Sha256, and Actual_Md5 to every matching dependency and increment resolved appropriately.
587-603: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCheck
reader.GetError()after the AQL record loop.
NextRecordreturns nil on completion or an actual read error; callers should callreader.GetError()after the loop. IfGetError()is non-nil, log the enrichment failure instead of presenting partial enrichment as success.🤖 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/alpine/apkcommand.go` around lines 587 - 603, After the record loop in the AQL enrichment flow, call reader.GetError() and handle any non-nil error by logging the enrichment failure instead of reporting partial results as successful. Keep the existing resolved-count success log only for error-free completion, using the surrounding enrichment function and reader loop as the change location.artifactory/commands/alpine/apkupload_test.go (2)
145-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for
enrichDepsFromLocalCache.This test exercises
collectApkDependenciesend-to-end but doesn't populate/var/cache/apkorAPKCACHE, soenrichDepsFromLocalCache— the local-cache checksum lookup that replaced the previously-flagged per-dependencyapk fetchapproach — has no assertion coverage on the success path (checksum actually populated from a cached file). Consider a focused test that setsAPKCACHEto a temp dir containing a fixture.apkand assertsSha1/Sha256get populated on the matching dependency.🤖 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/alpine/apkupload_test.go` around lines 145 - 162, Add focused unit coverage for enrichDepsFromLocalCache rather than relying only on TestCollectApkDependencies_ScopesMatchAddFlow. Configure APKCACHE to a temporary directory containing a matching fixture .apk, invoke the cache-enrichment path with the corresponding dependency, and assert that both Sha1 and Sha256 are populated from the cached file.
18-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for the standalone
"."value.
validateArtifactoryPathSegmentexplicitly rejectsvalue == "."(a separate branch from the".."/slash checks). None of the current test cases exercise this specific value. Add a{name: "single dot", value: ".", wantErr: true}case to cover it directly.🤖 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/alpine/apkupload_test.go` around lines 18 - 43, Add a `"single dot"` table entry with value `"."` and wantErr set to true in TestValidateArtifactoryPathSegment, covering the validator’s explicit standalone-dot rejection.artifactory/commands/alpine/apkupload.go (3)
29-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a non-zero retry backoff for uploads.
uploadHTTPRetryWaitMilliSecsis0.CreateUploadServiceManager(Line 245-246) uses this foruploadHTTPRetries = 3retries with no wait. On a transient failure or rate-limit response, this retries immediately three times with no backoff. In CI pipelines that invokejf apk uploadmany times in sequence or parallel, this increases load on Artifactory during an outage instead of giving it time to recover.Set a non-zero wait between retries.
♻️ Proposed fix
const ( uploadThreads = 1 uploadHTTPRetries = 3 - uploadHTTPRetryWaitMilliSecs = 0 + uploadHTTPRetryWaitMilliSecs = 1000 )🤖 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/alpine/apkupload.go` around lines 29 - 33, Update the uploadHTTPRetryWaitMilliSecs constant used by CreateUploadServiceManager to a non-zero delay, preserving the existing three-retry behavior while adding backoff between upload retries.
310-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
artifactoryPathinstead of rebuilding the same path string.
artifactoryPath(Line 321) andArtifacts[0].Path(Line 332) are built from the same format string and the same fields; a third build of the identical string exists at Line 220 for the uploadtarget. If the path layout ever changes, one of these Sprintf calls can drift from the others and silently desync the recorded Build Info artifact path, the dependencyrequestedBypath, and the actual upload target.Reuse the already-computed
artifactoryPathforPath.♻️ Proposed fix
Artifacts: []entities.Artifact{{ Name: fmt.Sprintf("%s:%s:%s", pkgName, pkgVersion, arch), - Path: fmt.Sprintf("%s/%s/%s/%s/%s", apkCmd.repoKey, apkCmd.alpineVersion, apkCmd.branch, arch, filename), + Path: artifactoryPath,🤖 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/alpine/apkupload.go` around lines 310 - 343, In recordBuildInfoArtifact, reuse the existing artifactoryPath value for the artifact’s entities.Artifact.Path instead of rebuilding the identical path with fmt.Sprintf. Keep artifactoryPath as the single source for the dependency requestedBy path and recorded artifact path.
439-475: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the
apk infofallback instead of resolving one dependency per process.
depsFromApkInfoCommandalready supports multiple package names, but every unresolved provider still callsresolveDepID, which spawns oneapk infosubprocess. Collect unresolved dependency IDs and pass them together instead of making N subprocess calls for one package.🤖 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/alpine/apkupload.go` around lines 439 - 475, The collectApkDependencies flow currently resolves each dependency individually, causing one apk info subprocess per unresolved provider. Update resolveDepIDWithProviders usage to collect unresolved dependency IDs during the loop, then batch-resolve them through the existing multi-package depsFromApkInfoCommand capability and apply the results back to the corresponding dependencies, preserving deduplication and existing provider resolution 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/alpine/apkcommand.go`:
- Around line 438-445: Update the apk command execution error handling around
cmd.Run() and exitErr.ExitCode() to normalize signal termination: when
ExitCode() returns -1, return a conventional nonzero exit status instead of
propagating -1, while preserving existing exit codes and stderr signature-hint
emission for regular process failures.
In `@artifactory/commands/setup/setup.go`:
- Around line 930-952: Update apkUpdateRepositories to track whether
apkRepositoriesFile existed separately from originalContent, using the
os.ReadFile result before replacing the not-exist error. In the write-failure
recovery branch, use that existence flag to attempt restoration even when the
prior file content was empty, while preserving the current behavior for files
that did not exist.
---
Nitpick comments:
In `@artifactory/commands/alpine/apkcommand_test.go`:
- Around line 208-211: Update TestEmitSignatureHint_DetectsPatterns to observe
emitSignatureHint output, capturing its log or using a testable return value,
and assert that the UNTRUSTED signature input produces a hint while the ordinary
input produces none.
In `@artifactory/commands/alpine/apkcommand.go`:
- Around line 565-599: Update the checksum enrichment flow around fileToDep and
the reader.NextRecord loop to retain all dependency indices for each file name
rather than overwriting duplicates. Map each item.Name to its slice of indices,
then apply Actual_Sha1, Sha256, and Actual_Md5 to every matching dependency and
increment resolved appropriately.
- Around line 587-603: After the record loop in the AQL enrichment flow, call
reader.GetError() and handle any non-nil error by logging the enrichment failure
instead of reporting partial results as successful. Keep the existing
resolved-count success log only for error-free completion, using the surrounding
enrichment function and reader loop as the change location.
In `@artifactory/commands/alpine/apkupload_test.go`:
- Around line 145-162: Add focused unit coverage for enrichDepsFromLocalCache
rather than relying only on TestCollectApkDependencies_ScopesMatchAddFlow.
Configure APKCACHE to a temporary directory containing a matching fixture .apk,
invoke the cache-enrichment path with the corresponding dependency, and assert
that both Sha1 and Sha256 are populated from the cached file.
- Around line 18-43: Add a `"single dot"` table entry with value `"."` and
wantErr set to true in TestValidateArtifactoryPathSegment, covering the
validator’s explicit standalone-dot rejection.
In `@artifactory/commands/alpine/apkupload.go`:
- Around line 29-33: Update the uploadHTTPRetryWaitMilliSecs constant used by
CreateUploadServiceManager to a non-zero delay, preserving the existing
three-retry behavior while adding backoff between upload retries.
- Around line 310-343: In recordBuildInfoArtifact, reuse the existing
artifactoryPath value for the artifact’s entities.Artifact.Path instead of
rebuilding the identical path with fmt.Sprintf. Keep artifactoryPath as the
single source for the dependency requestedBy path and recorded artifact path.
- Around line 439-475: The collectApkDependencies flow currently resolves each
dependency individually, causing one apk info subprocess per unresolved
provider. Update resolveDepIDWithProviders usage to collect unresolved
dependency IDs during the loop, then batch-resolve them through the existing
multi-package depsFromApkInfoCommand capability and apply the results back to
the corresponding dependencies, preserving deduplication and existing provider
resolution 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: a4f9d3cb-66fe-46ee-ba68-ec304b6447f1
📒 Files selected for processing (6)
artifactory/commands/alpine/apkcommand.goartifactory/commands/alpine/apkcommand_test.goartifactory/commands/alpine/apkupload.goartifactory/commands/alpine/apkupload_test.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
artifactory/commands/setup/setup.go (1)
683-705: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe raw
http.DefaultClientcalls still bypass the configured Artifactory client.
apkValidateRepositoryExists,apkFetchKeyPairRef, andapkDownloadRSAKeyusehttp.DefaultClient, which has no timeout and ignores configured proxy, TLS, retry, and auth settings. A stalled endpoint hangsjf setup apkindefinitely. This was reported before and marked as addressed, but the code still useshttp.DefaultClient.🤖 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/setup/setup.go` around lines 683 - 705, The APK setup HTTP helpers apkValidateRepositoryExists, apkFetchKeyPairRef, and apkDownloadRSAKey must stop using http.DefaultClient. Route their requests through the configured Artifactory client used by the surrounding setup flow so configured timeout, proxy, TLS, retry, and authentication behavior is preserved.
🧹 Nitpick comments (1)
artifactory/commands/alpine/apkcommand.go (1)
569-583: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe AQL query is not chunked.
All missing dependencies are placed in a single
$orlist. A largeapk addwith many uncached packages produces one very large query string. Artifactory can reject or slow down such requests. Splitmissinginto fixed-size chunks, for example 100 items, and merge the results.🤖 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/alpine/apkcommand.go` around lines 569 - 583, Update enrichDepsChecksumsFromAQL to process missing dependencies in fixed-size chunks of about 100 items, building and executing a separate AQL query for each chunk. Merge each chunk’s checksum results into the returned dependencies while preserving the existing repository filtering and behavior for all missing entries.
🤖 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/alpine/apkcommand.go`:
- Around line 174-178: Update extractPackageNames and its callers in the apk
command flow so values belonging to native value-taking flags such as
--cache-dir, --repository, and -t are not classified as requested packages.
Ensure the filtered package list remains correct for excludeRequestedPackages,
alpineModule.SetRequestedPackages, and the completeness warning while preserving
genuine package arguments.
---
Duplicate comments:
In `@artifactory/commands/setup/setup.go`:
- Around line 683-705: The APK setup HTTP helpers apkValidateRepositoryExists,
apkFetchKeyPairRef, and apkDownloadRSAKey must stop using http.DefaultClient.
Route their requests through the configured Artifactory client used by the
surrounding setup flow so configured timeout, proxy, TLS, retry, and
authentication behavior is preserved.
---
Nitpick comments:
In `@artifactory/commands/alpine/apkcommand.go`:
- Around line 569-583: Update enrichDepsChecksumsFromAQL to process missing
dependencies in fixed-size chunks of about 100 items, building and executing a
separate AQL query for each chunk. Merge each chunk’s checksum results into the
returned dependencies while preserving the existing repository filtering and
behavior for all missing entries.
🪄 Autofix
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: cbe31892-145e-43d2-8f02-09d5e9459d24
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
artifactory/commands/alpine/apkcommand.goartifactory/commands/alpine/apkcommand_test.goartifactory/commands/alpine/apkupload.goartifactory/commands/alpine/apkupload_test.goartifactory/commands/alpine/credentials.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- artifactory/commands/alpine/credentials.go
- artifactory/commands/alpine/apkupload_test.go
- artifactory/commands/setup/setup_test.go
- artifactory/commands/alpine/apkcommand_test.go
- artifactory/commands/alpine/apkupload.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
artifactory/commands/alpine/apkcommand.go (2)
469-473: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the resolved
apkPathfor the version probe.
Runresolves the binary withexec.LookPath("apk")at Line 162, thenwarnIfApkTooOldperforms a second PATH lookup. PassapkPathintowarnIfApkTooOldso the version check inspects the same binary that will be executed.🤖 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/alpine/apkcommand.go` around lines 469 - 473, Update warnIfApkTooOld to accept the resolved apkPath and use it when invoking the version command instead of looking up "apk" again. Pass the apkPath resolved by Run into warnIfApkTooOld so both checks inspect the same binary.
296-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the secret env filter on the early-return paths too.
filterSecretEnvVarsruns only after thertURL == ""early returns at Lines 292 and 298. When no server is configured, the apk subprocess receives the unfiltered environment. If the intent is "never pass secret-like variables to apk", move the filter above the early returns so all paths return the same filtered environment.♻️ Proposed change
func (apkCmd *ApkCommand) buildEnvWithHTTPAuth() ([]string, error) { - env := os.Environ() + // Filter out env vars matching the JFROG_CLI_ENV_EXCLUDE pattern to avoid secret leaks + // in the subprocess environment. + env := filterSecretEnvVars(os.Environ()) @@ if rtURL == "" { log.Warn("No JFrog server configured — skipping HTTP_AUTH injection. Run: jf c add") return env, nil } - - // Filter out env vars matching the JFROG_CLI_ENV_EXCLUDE pattern to avoid secret leaks - // in the subprocess environment. - env = filterSecretEnvVars(env)🤖 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/alpine/apkcommand.go` around lines 296 - 303, Move the filterSecretEnvVars call above the rtURL == "" early-return branches in the APK environment setup so every return path, including no configured server, returns a sanitized environment. Preserve the existing warning and return behavior while ensuring the apk subprocess never receives unfiltered secret-like variables.
🤖 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/setup/setup.go`:
- Around line 1096-1102: Validate keyPairRef in apkWriteSigningKey before
constructing keyFilePath with filepath.Join, rejecting values containing '/',
'\', or '..'; alternatively use only the validated base name. Ensure invalid
references return an error and cannot cause apkWriteFile to target outside
apkKeysDir.
---
Nitpick comments:
In `@artifactory/commands/alpine/apkcommand.go`:
- Around line 469-473: Update warnIfApkTooOld to accept the resolved apkPath and
use it when invoking the version command instead of looking up "apk" again. Pass
the apkPath resolved by Run into warnIfApkTooOld so both checks inspect the same
binary.
- Around line 296-303: Move the filterSecretEnvVars call above the rtURL == ""
early-return branches in the APK environment setup so every return path,
including no configured server, returns a sanitized environment. Preserve the
existing warning and return behavior while ensuring the apk subprocess never
receives unfiltered secret-like variables.
🪄 Autofix
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: 413395c4-20be-4b8b-825a-b6c9cc9c4fe4
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
artifactory/commands/alpine/apkcommand.goartifactory/commands/alpine/apkcommand_test.goartifactory/commands/alpine/apkupload.goartifactory/commands/alpine/apkupload_test.goartifactory/commands/alpine/credentials.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (4)
- artifactory/commands/alpine/credentials.go
- artifactory/commands/alpine/apkupload_test.go
- artifactory/commands/alpine/apkupload.go
- artifactory/commands/alpine/apkcommand_test.go
| if err = apkMkdirAll(apkKeysDir); err != nil { | ||
| return fmt.Errorf("failed to create %s: %w", apkKeysDir, err) | ||
| } | ||
| keyFilePath := filepath.Join(apkKeysDir, keyPairRef+".rsa.pub") | ||
| if err = apkWriteFile(keyFilePath, pemKey, 0644); err != nil { | ||
| return fmt.Errorf("failed to write RSA key to %s: %w", keyFilePath, err) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm keyPairRef flows unvalidated from the API response into the write path.
rg -n -C4 'primaryKeyPairRef|keyPairRef' artifactory/commands/setup/setup.go
rg -n -C4 'func apkWriteFile' artifactory/commands/setup/setup.goRepository: jfrog/jfrog-cli-artifactory
Length of output: 2689
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Relevant setup.go sections =="
sed -n '1081,1160p' artifactory/commands/setup/setup.go
sed -n '1197,1218p' artifactory/commands/setup/setup.go
echo
echo "== Existing sanitization/helper references nearby =="
rg -n 'validateArtifactoryPathSegment|filepath\.Base|strings\.ContainsAny|apkKeysDir|apkMkdirAll|apkWriteFile' artifactory/commands/setup/setup.go
echo
echo "== Static verifier: path traversal invariant from API response to APK key directory =="
python3 - <<'PY'
import subprocess
p = subprocess.run(["go", "tool", "asm", "-c", "filepath.Join", "/tmp/x.go"], cwd="/tmp", stdout=subprocess.PIPE, stderr=subprocess.PIPE, env={**subprocess.os.environ})
print("go asm result:", p.returncode)
PY
echo
echo "== Run-only behavioral probe for Go filepath.Join in this tree =="
go env GOROOT 2>/dev/null | sed -n '1p' || true
tmpdir="$(mktemp -d)"
cat > "$tmpdir/main.go" <<'GO'
package main
import (
"path/filepath"
"fmt"
)
func main() {
for _, keyPairRef := range []string{"../../../etc/cron.d/x", "../evil", "safe", "foo/bar"} {
keyFilePath := filepath.Join("/etc/apk/keys", keyPairRef+".rsa.pub")
fmt.Printf("%q -> %q\n", keyPairRef, keyFilePath)
}
}
GO
go run "$tmpdir/main.go"
rm -rf "$tmpdir"Repository: jfrog/jfrog-cli-artifactory
Length of output: 5670
Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability: Internal
Reachability path
● Entry
artifactory/commands/setup/setup_test.go:1281
TestApkValidateRepositoryExists
│
▼
● Sink
artifactory/commands/setup/setup.go
Validate keyPairRef before it becomes a filesystem path.
apkFetchKeyPairRef returns primaryKeyPairRef directly, and apkWriteSigningKey appends it to /etc/apk/keys with filepath.Join. Joined components are cleaned, so ../../../etc/cron.d/x resolves outside the intended directory; apkWriteFile then writes that path through sudo. Reject any keyPairRef containing /, \, or .., or insert only its base name.
🤖 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/setup/setup.go` around lines 1096 - 1102, Validate
keyPairRef in apkWriteSigningKey before constructing keyFilePath with
filepath.Join, rejecting values containing '/', '\', or '..'; alternatively use
only the validated base name. Ensure invalid references return an error and
cannot cause apkWriteFile to target outside apkKeysDir.
Source: Linters/SAST tools
Adds native Alpine Linux APK support to jfrog-cli-artifactory: - artifactory/commands/alpine: the jf apk wrapper that runs the native apk binary against an Artifactory-backed repository, collecting Build Info via build-info-go AlpineModule (install-side dependency graph with checksums, scopes and requestedBy chains) and uploading downloaded archives. - artifactory/commands/setup: jf setup apk configuration - writes the repo definition and signing key to the apk config, creating credential-bearing files owner-only (umask 077) and restoring prior content on failure. - go.mod: bump build-info-go and jfrog-cli-core to the commits carrying the Alpine build-info APIs (SetDownloadsDir, apk module) and the apk ProjectType. Co-authored-by: Cursor <cursoragent@cursor.com>
Run() called ensureRepoExists unconditionally whenever --repo was set, even with no JFrog server configured at all. With no server there is nothing to validate against, and apk still works via the system's default repositories in that case (matching upload's existing guard).
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
artifactory/commands/setup/setup.go (2)
1096-1102: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate
keyPairRefbefore it becomes a filesystem path.
apkFetchKeyPairRefreturnsprimaryKeyPairReffrom the API response without validation.filepath.Joincleans the joined path, so a value such as../../../etc/cron.d/xresolves outside/etc/apk/keys, andapkWriteFilethen writes that path throughsudo. Reject any value that contains/,\, or.., or use only its base name.🛡️ Proposed fix
+ if strings.ContainsAny(keyPairRef, `/\`) || strings.Contains(keyPairRef, "..") { + return fmt.Errorf("invalid primaryKeyPairRef %q returned for repo %q", keyPairRef, repoKey) + } if err = apkMkdirAll(apkKeysDir); err != nil { return fmt.Errorf("failed to create %s: %w", apkKeysDir, err) } keyFilePath := filepath.Join(apkKeysDir, keyPairRef+".rsa.pub")🤖 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/setup/setup.go` around lines 1096 - 1102, Validate keyPairRef immediately after apkFetchKeyPairRef returns and before filepath.Join in the setup flow, rejecting values containing "/", "\", or ".." (or normalizing to a safe base name). Ensure invalid references return an error and only validated values reach apkWriteFile for paths under apkKeysDir.
975-997: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
http.DefaultClientstill bypasses the configured Artifactory client.
apkValidateRepositoryExists(Line 983),apkFetchKeyPairRef(Line 1129), andapkDownloadRSAKey(Line 1166) all usehttp.DefaultClientwith a hand-builtAuthorizationheader. That client has no timeout, so a stalled Artifactory endpoint hangsjf setup apkindefinitely. It also ignores the configured proxy, TLS/insecure settings, retries, and any non-basic/non-bearer auth scheme. Route these calls throughartutils.CreateServiceManager(for exampleIsRepoExists) or, at minimum, through a client with an explicit timeout and the configured transport.This was raised on the previous commit and marked as addressed, but the code still calls
http.DefaultClient.🤖 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/setup/setup.go` around lines 975 - 997, Replace the direct http.DefaultClient usage in apkValidateRepositoryExists, apkFetchKeyPairRef, and apkDownloadRSAKey with the configured Artifactory client via artutils.CreateServiceManager, reusing its repository-existence and request capabilities where applicable. Preserve the existing validation and error behavior while ensuring configured authentication, proxy, TLS, retries, and timeout settings are honored.
🤖 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/alpine/apkcommand.go`:
- Around line 709-729: Update extractPackageNames to normalize each non-flag
package token by removing its version constraint, including =, >, and ~
specifiers, before appending it to pkgs. Preserve existing flag skipping and
empty-token handling so returned values contain only package names for
excludeRequestedPackages and SetRequestedPackages.
- Around line 303-320: Update the existing HTTP_AUTH-preservation branch in the
environment-building function to restore the pre-existing HTTP_AUTH value to the
filtered env before returning. Keep the current behavior of honoring the
original value when no explicit username/password flags are provided, while
retaining override behavior for explicit flags.
In `@artifactory/commands/setup/setup.go`:
- Around line 1272-1278: Update apkMergeRepositoriesContent to match existing
repository lines by scheme, host, and repository path prefix rather than
hostname alone, so distinct branches such as main and community from the same
Artifactory host are preserved. Keep replacement limited to the intended
repository entry while retaining insertion behavior for the configured main
branch.
- Around line 951-958: Update the Alpine repository URL setup around
detectAlpineVersion so that when it returns an empty string, the existing
fallback URL path is retained but a warning is logged explaining that the Alpine
release could not be detected and the repository URL may be invalid. Use the
surrounding setup logger and keep the detected-version URL behavior unchanged.
---
Duplicate comments:
In `@artifactory/commands/setup/setup.go`:
- Around line 1096-1102: Validate keyPairRef immediately after
apkFetchKeyPairRef returns and before filepath.Join in the setup flow, rejecting
values containing "/", "\", or ".." (or normalizing to a safe base name). Ensure
invalid references return an error and only validated values reach apkWriteFile
for paths under apkKeysDir.
- Around line 975-997: Replace the direct http.DefaultClient usage in
apkValidateRepositoryExists, apkFetchKeyPairRef, and apkDownloadRSAKey with the
configured Artifactory client via artutils.CreateServiceManager, reusing its
repository-existence and request capabilities where applicable. Preserve the
existing validation and error behavior while ensuring configured authentication,
proxy, TLS, retries, and timeout settings are honored.
🪄 Autofix
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: 901f1b99-f3db-4f28-9071-06321cfceca1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
artifactory/commands/alpine/apkcommand.goartifactory/commands/alpine/apkcommand_test.goartifactory/commands/alpine/apkupload.goartifactory/commands/alpine/apkupload_test.goartifactory/commands/alpine/credentials.goartifactory/commands/setup/setup.goartifactory/commands/setup/setup_test.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (6)
- go.mod
- artifactory/commands/alpine/credentials.go
- artifactory/commands/alpine/apkupload_test.go
- artifactory/commands/alpine/apkupload.go
- artifactory/commands/setup/setup_test.go
- artifactory/commands/alpine/apkcommand_test.go
| // Filter out env vars matching the JFROG_CLI_ENV_EXCLUDE pattern to avoid secret leaks | ||
| // in the subprocess environment. | ||
| env = filterSecretEnvVars(env) | ||
|
|
||
| // Explicit --user/--password flags override any pre-set HTTP_AUTH; a stored/default | ||
| // server config does not, so a user-provided HTTP_AUTH is otherwise honoured as-is. | ||
| userExplicitFlags := apkCmd.username != "" || apkCmd.password != "" | ||
|
|
||
| existingHTTPAuth := os.Getenv("HTTP_AUTH") | ||
| if existingHTTPAuth != "" { | ||
| if userExplicitFlags { | ||
| log.Warn("HTTP_AUTH is already set in your environment. Overriding it with the credentials from the provided flags/server config.") | ||
| } else { | ||
| // User did not pass explicit flags — honour their pre-set HTTP_AUTH. | ||
| log.Debug("HTTP_AUTH already set in environment and no explicit flags provided — keeping existing value.") | ||
| return env, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The "keep existing HTTP_AUTH" branch returns an environment with HTTP_AUTH removed.
filterSecretEnvVars at Line 305 drops every variable whose lowercased name matches *auth*, and http_auth matches that pattern. On the branch at Lines 316-318 the function returns the filtered env without re-adding the value. The user's pre-set HTTP_AUTH is therefore silently lost, and apk runs unauthenticated instead of honouring it.
🐛 Proposed fix
env = filterSecretEnvVars(env)
// Explicit --user/--password flags override any pre-set HTTP_AUTH; a stored/default
// server config does not, so a user-provided HTTP_AUTH is otherwise honoured as-is.
userExplicitFlags := apkCmd.username != "" || apkCmd.password != ""
existingHTTPAuth := os.Getenv("HTTP_AUTH")
if existingHTTPAuth != "" {
if userExplicitFlags {
log.Warn("HTTP_AUTH is already set in your environment. Overriding it with the credentials from the provided flags/server config.")
} else {
// User did not pass explicit flags — honour their pre-set HTTP_AUTH.
log.Debug("HTTP_AUTH already set in environment and no explicit flags provided — keeping existing value.")
- return env, nil
+ // filterSecretEnvVars removed HTTP_AUTH (it matches *auth*), so re-add it.
+ return append(env, "HTTP_AUTH="+existingHTTPAuth), nil
}
}📝 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.
| // Filter out env vars matching the JFROG_CLI_ENV_EXCLUDE pattern to avoid secret leaks | |
| // in the subprocess environment. | |
| env = filterSecretEnvVars(env) | |
| // Explicit --user/--password flags override any pre-set HTTP_AUTH; a stored/default | |
| // server config does not, so a user-provided HTTP_AUTH is otherwise honoured as-is. | |
| userExplicitFlags := apkCmd.username != "" || apkCmd.password != "" | |
| existingHTTPAuth := os.Getenv("HTTP_AUTH") | |
| if existingHTTPAuth != "" { | |
| if userExplicitFlags { | |
| log.Warn("HTTP_AUTH is already set in your environment. Overriding it with the credentials from the provided flags/server config.") | |
| } else { | |
| // User did not pass explicit flags — honour their pre-set HTTP_AUTH. | |
| log.Debug("HTTP_AUTH already set in environment and no explicit flags provided — keeping existing value.") | |
| return env, nil | |
| } | |
| } | |
| // Filter out env vars matching the JFROG_CLI_ENV_EXCLUDE pattern to avoid secret leaks | |
| // in the subprocess environment. | |
| env = filterSecretEnvVars(env) | |
| // Explicit --user/--password flags override any pre-set HTTP_AUTH; a stored/default | |
| // server config does not, so a user-provided HTTP_AUTH is otherwise honoured as-is. | |
| userExplicitFlags := apkCmd.username != "" || apkCmd.password != "" | |
| existingHTTPAuth := os.Getenv("HTTP_AUTH") | |
| if existingHTTPAuth != "" { | |
| if userExplicitFlags { | |
| log.Warn("HTTP_AUTH is already set in your environment. Overriding it with the credentials from the provided flags/server config.") | |
| } else { | |
| // User did not pass explicit flags — honour their pre-set HTTP_AUTH. | |
| log.Debug("HTTP_AUTH already set in environment and no explicit flags provided — keeping existing value.") | |
| // filterSecretEnvVars removed HTTP_AUTH (it matches *auth*), so re-add it. | |
| return append(env, "HTTP_AUTH="+existingHTTPAuth), nil | |
| } | |
| } |
🤖 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/alpine/apkcommand.go` around lines 303 - 320, Update the
existing HTTP_AUTH-preservation branch in the environment-building function to
restore the pre-existing HTTP_AUTH value to the filtered env before returning.
Keep the current behavior of honoring the original value when no explicit
username/password flags are provided, while retaining override behavior for
explicit flags.
| func extractPackageNames(args []string) []string { | ||
| var pkgs []string | ||
| skipNext := false | ||
| for _, arg := range args { | ||
| if skipNext { | ||
| skipNext = false | ||
| continue | ||
| } | ||
| if strings.HasPrefix(arg, "-") { | ||
| // `--flag=value` carries its own value; `--flag value` consumes the next token. | ||
| if !strings.Contains(arg, "=") && apkValueFlags[arg] { | ||
| skipNext = true | ||
| } | ||
| continue | ||
| } | ||
| if arg != "" { | ||
| pkgs = append(pkgs, arg) | ||
| } | ||
| } | ||
| return pkgs | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Strip version constraints from requested package tokens.
apk add accepts constrained specifiers such as curl=8.5.0-r0, curl>1.0, and curl~3.2. extractPackageNames returns the whole token, so excludeRequestedPackages never matches pkg.Name, and SetRequestedPackages records a value that is not a package name. Normalize each token to the name part.
🩹 Proposed fix
if arg != "" {
- pkgs = append(pkgs, arg)
+ pkgs = append(pkgs, trimPkgConstraint(arg))
}
}
return pkgs
}
+
+// trimPkgConstraint reduces "curl=8.5.0-r0", "curl>1.0", or "curl~3.2" to "curl".
+func trimPkgConstraint(arg string) string {
+ if idx := strings.IndexAny(arg, "=<>~"); idx > 0 {
+ return arg[:idx]
+ }
+ return arg
+}📝 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 extractPackageNames(args []string) []string { | |
| var pkgs []string | |
| skipNext := false | |
| for _, arg := range args { | |
| if skipNext { | |
| skipNext = false | |
| continue | |
| } | |
| if strings.HasPrefix(arg, "-") { | |
| // `--flag=value` carries its own value; `--flag value` consumes the next token. | |
| if !strings.Contains(arg, "=") && apkValueFlags[arg] { | |
| skipNext = true | |
| } | |
| continue | |
| } | |
| if arg != "" { | |
| pkgs = append(pkgs, arg) | |
| } | |
| } | |
| return pkgs | |
| } | |
| func extractPackageNames(args []string) []string { | |
| var pkgs []string | |
| skipNext := false | |
| for _, arg := range args { | |
| if skipNext { | |
| skipNext = false | |
| continue | |
| } | |
| if strings.HasPrefix(arg, "-") { | |
| // `--flag=value` carries its own value; `--flag value` consumes the next token. | |
| if !strings.Contains(arg, "=") && apkValueFlags[arg] { | |
| skipNext = true | |
| } | |
| continue | |
| } | |
| if arg != "" { | |
| pkgs = append(pkgs, trimPkgConstraint(arg)) | |
| } | |
| } | |
| return pkgs | |
| } | |
| // trimPkgConstraint reduces "curl=8.5.0-r0", "curl>1.0", or "curl~3.2" to "curl". | |
| func trimPkgConstraint(arg string) string { | |
| if idx := strings.IndexAny(arg, "=<>~"); idx > 0 { | |
| return arg[:idx] | |
| } | |
| return arg | |
| } |
🤖 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/alpine/apkcommand.go` around lines 709 - 729, Update
extractPackageNames to normalize each non-flag package token by removing its
version constraint, including =, >, and ~ specifiers, before appending it to
pkgs. Preserve existing flag skipping and empty-token handling so returned
values contain only package names for excludeRequestedPackages and
SetRequestedPackages.
| alpineVersion := detectAlpineVersion() | ||
|
|
||
| var repoURL string | ||
| if alpineVersion != "" { | ||
| repoURL = fmt.Sprintf("%s/%s/%s/%s/", rtURL, sc.repoName, alpineVersion, apkDefaultBranch) | ||
| } else { | ||
| repoURL = fmt.Sprintf("%s/%s/", rtURL, sc.repoName) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Warn when the Alpine release cannot be detected.
If /etc/alpine-release is missing or unparseable, detectAlpineVersion returns "" and the code writes <rtURL>/<repo>/ with no version or branch segment. apk cannot fetch APKINDEX.tar.gz from that URL, so the setup reports success and the next apk command fails with an unclear error. Log a warning on this path.
🩹 Proposed fix
var repoURL string
if alpineVersion != "" {
repoURL = fmt.Sprintf("%s/%s/%s/%s/", rtURL, sc.repoName, alpineVersion, apkDefaultBranch)
} else {
+ log.Warn(fmt.Sprintf("Could not determine the Alpine release from %s. The repository URL is written without a "+
+ "version and branch segment, so apk may not find an index. Run this command on an Alpine host, or edit %s manually.",
+ alpineReleaseFile, apkRepositoriesFile))
repoURL = fmt.Sprintf("%s/%s/", rtURL, sc.repoName)
}📝 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.
| alpineVersion := detectAlpineVersion() | |
| var repoURL string | |
| if alpineVersion != "" { | |
| repoURL = fmt.Sprintf("%s/%s/%s/%s/", rtURL, sc.repoName, alpineVersion, apkDefaultBranch) | |
| } else { | |
| repoURL = fmt.Sprintf("%s/%s/", rtURL, sc.repoName) | |
| } | |
| alpineVersion := detectAlpineVersion() | |
| var repoURL string | |
| if alpineVersion != "" { | |
| repoURL = fmt.Sprintf("%s/%s/%s/%s/", rtURL, sc.repoName, alpineVersion, apkDefaultBranch) | |
| } else { | |
| log.Warn(fmt.Sprintf("Could not determine the Alpine release from %s. The repository URL is written without a "+ | |
| "version and branch segment, so apk may not find an index. Run this command on an Alpine host, or edit %s manually.", | |
| alpineReleaseFile, apkRepositoriesFile)) | |
| repoURL = fmt.Sprintf("%s/%s/", rtURL, sc.repoName) | |
| } |
🤖 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/setup/setup.go` around lines 951 - 958, Update the
Alpine repository URL setup around detectAlpineVersion so that when it returns
an empty string, the existing fallback URL path is retained but a warning is
logged explaining that the Alpine release could not be detected and the
repository URL may be invalid. Use the surrounding setup logger and keep the
detected-version URL behavior unchanged.
| if artHost != "" && apkRepoHostname(trimmed) == artHost { | ||
| if !inserted { | ||
| out = append(out, repoURL) | ||
| inserted = true | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Host-level matching removes every other repository line from the same Artifactory host.
apkMergeRepositoriesContent compares only the hostname. A user who has both .../v3.20/main/ and .../v3.20/community/ from the same Artifactory host keeps only the new line, so apk loses access to the community branch. configureApk always writes the main branch, so there is no way to keep a second branch after setup.
Consider matching on scheme+host+path prefix instead of hostname alone, or logging the lines that are replaced so the change is visible to the user.
🤖 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/setup/setup.go` around lines 1272 - 1278, Update
apkMergeRepositoriesContent to match existing repository lines by scheme, host,
and repository path prefix rather than hostname alone, so distinct branches such
as main and community from the same Artifactory host are preserved. Keep
replacement limited to the intended repository entry while retaining insertion
behavior for the configured main branch.
2 participants
Summary
Implements the JFrog CLI Artifactory layer for Alpine APK integration:
jf apk add,jf apk upload, andjf apk config. This layer sits between the JFrog CLI frontend (jfrog-cli) and the build-info library (build-info-go), handling credential injection, build-info orchestration, AQL checksum enrichment, and repository configuration.Depends on: build-info-go#393
What Changed
New:
artifactory/commands/alpine/apkcommand.goThe main
jf apk <subcommand>wrapper:ApkCommand— wraps the nativeapkbinary with credential injection and Build Info collectionapk, diffs after to identify newly installed packagesbuildEnvWithHTTPAuth()— injectsHTTP_AUTH=basic:<host>:<user>:<pass>into the subprocess environment soapk-tools ≥ 2.12can authenticate against Artifactory virtual repositories without touching/etc/apk/repositoriescollectBuildInfo()— two-phase collection: local checksums first (from APK DBC:field), then AQL enrichment for SHA-256/MD5 of packages that went through ArtifactoryenrichChecksumsFromAQL()— batched AQL query against the specified virtual repo to resolve SHA-256, SHA-1, and MD5 for any deps with missing checksumsstripJFFlags()/extractPackageNames()— strips JFrog-specific flags (--repo,--build-name, etc.) before forwarding args to nativeapkfilterSecretEnvVars()— respectsJFROG_CLI_ENV_EXCLUDEto prevent secrets leaking into the subprocess environment or build-info recordsNew:
artifactory/commands/alpine/apkupload.goPublishes a local
.apkfile to an Artifactory Alpine repository:<repo>/<branch>/<arch>/<name>-<version>.apk)--branch,--archoverridesNew:
artifactory/commands/alpine/apkconfig.goConfigures the local Alpine client to use Artifactory as the package source:
/etc/apk/keys//etc/apk/repositoriesto replace CDN URLs with the Artifactory virtual repository URL--applyflag controls whether changes are written to disk or previewed/etc/apk/repositorieson write failure to avoid leaving the system in a broken stateNew:
artifactory/commands/alpine/credentials.goCredential resolution helpers:
resolveHTTPAuthCredentials()— merges server config, explicit--user/--passwordflags, and token-based authbuildHTTPAuth()— formats theHTTP_AUTH=basic:<host>:<user>:<pass>string forapk-toolsDesign Notes
No routing change without
jf apk config:jf apk add --repo=Xalone does not change/etc/apk/repositories. Packages are fetched from whereverapkis currently configured (CDN by default). The--repoflag affects HTTP_AUTH injection and AQL enrichment only. This is intentional — routing all traffic through Artifactory requires an explicitjf apk configstep, matching the security model of theaptandyumintegrations.--repois optional for build-info: Consistent withjf npm installandjf pip install, omitting--repodoes not block build-info collection. Dependencies are still recorded with checksums from the APK DB. AQL enrichment (for SHA-256/MD5) is simply skipped.Unknown
--server-idis a hard error: If--server-idis explicitly provided and the server is not in the config, the command fails immediately — consistent with every other JFrog CLI package manager command.Dependency ID format:
name:version(e.g.curl:8.14.1-r2), consistent with npm, pip, and Maven modules.Summary by CodeRabbit
New Features
Tests