feat: Jfrog CLI for Artifactory SDK targets - #168
Conversation
MEverett90
changed the title
9702dca to
7dd650a
Compare
There was a problem hiding this comment.
The sh -> direct-exec change is correct — chmod +x already runs just above at line 220, so the shebang is honored — and threading a real reason into the unavailable message is a solid UX improvement over the bare "no environment-setup-* found". Three issues in the new jf path, inline.
One more small thing not worth its own thread: _downloadError is never cleared, so if resolve() is ever called twice on the same provider instance a stale reason can leak into an unrelated failure message.
…, select jf server-id by URL authority, fall through to HTTP when jf absent - Clear _downloadError and _artifacts at the top of resolve() so stale state from a prior call never leaks into an unrelated error message or lock entry. - _downloadViaJFrog now returns bool? (null = fall through to plain HTTP) and returns null in all cases where jf cannot be used: CLI not installed, `jf config show` fails, or no configured server matches the Artifactory URL's authority. - Resolves the correct --server-id by matching the Artifactory URL's authority against `jf config show` output, so multi-server setups pick the right credentials rather than silently using the default. - Adds --fail-no-op to jf rt dl and a post-download existence check so a silent mismatch between the URL basename and the repository artifact name produces a clear error instead of a missing-file mystery later. - Fixes _parseArtifactoryPath to use uri.authority (preserves port) instead of uri.host.
- Drop redundant jf --version probe; jf config show already throws ProcessException when jf is absent. - _parseArtifactoryPath now returns (authority, repoPath) directly, removing the dead basePath construction and the Uri.parse round-trip in _downloadViaJFrog. - Extract _failMsg helper to deduplicate the stderr-formatting pattern shared by _materializeFromUrl and _downloadViaJFrog. - Parse the installer URL once in _materializeFromUrl instead of twice. - Remove no-op value.trim() in _parseJFrogServers (line already trimmed before the regex match).
|
Good iteration. Confirming what the last two commits closed:
1. Still open from last round: the
This is worse now than when I first raised it, because the failure is no longer recoverable. 2. I said last time the final ProcessResult run;
try {
run = await Process.run(installer.path, ['-y', '-d', prefix.path]);
} on ProcessException catch (e) {
_downloadError = 'cannot execute ${installer.path}: ${e.message}';
return null;
}3. jf installed but no server matches -> the fallback reports the wrong cause. Keeping the HTTP fallback is right, but when it fires because no configured server matches the authority (line 301), the user sees _downloadError = 'no jf server configured for $baseAuthority — run `jf c add`';
return null;4. A truncated installer persists and is then executed.
5.
Nits:
|
…aths, and parser tests Download to a .part file and rename on success so a mid-stream connection drop cannot leave a truncated installer that silently re-used on the next run. Strip the api/download prefix from Artifactory paths before passing them to jf rt dl, fixing a regression where those URLs failed hard instead of falling back to HTTP. Expose parseArtifactoryPath and parseJFrogServers as @VisibleForTesting statics (with meta dep) and add tests covering plain, api/download, ported, and no-artifactory URL forms plus a real multi-server jf config show sample.
…ccuracy, Update spelling
|
@jwinarske Thanks for the follow-up, concerns have been addressed in latest commit, please re-review at your earliest convenience. |
There was a problem hiding this comment.
Read through the latest. The shape is good — resolving --server-id by matching
the URL authority against jf config show rather than assuming the default
server is the right call, the .part staging on the HTTP path is a real
improvement, and parseArtifactoryPath / parseJFrogServers being pure and
@visibleForTesting makes the tricky parts testable without a network. Running
the installer directly instead of through sh is correct, and chmod +x above
it already covers the exec bit.
Two things I would change, plus one consideration.
1. _failureDetail survives a successful fallback and can mislead
_downloadViaJFrog sets
_failureDetail = "no jf server configured for $baseAuthority — run `jf c add`";
return null;then returns null so the caller falls through to plain HTTP. _download never
clears it on the success path, and resolve() only reads it much later.
So: Artifactory URL, jf installed but no server matching that authority, HTTP
fallback downloads fine, installer runs, and the SDK layout turns out not to
have an environment-setup-* where expected. The user gets
no environment-setup-* found (no jf server configured for artifacts.example.com
— run `jf c add`) — set cross.sdk_path to ...
which points at JFrog config that was not the problem and had already been
routed around. One line fixes it — clear _failureDetail on the success paths
in _download, so a detail only ever describes the failure actually being
reported.
2. The jf path lacks the atomicity the HTTP path just gained
The HTTP path now writes ${dest.path}.part and renames, which is exactly the
right fix. jf rt dl --flat writes into dest.parent directly, so an
interrupted download (Ctrl-C, runner timeout, disk full) can leave a truncated
file at dest.path.
The next run then takes the installer.existsSync() branch, skips the download,
records that partial file's sha256 in emb.lock, chmods it and executes it. The
best case is a confusing installer failure; the worse case is a pinned lock
entry for a corrupt artifact. Downloading into a temp directory and renaming
into place would give jf the same guarantee — and it matters more here, since
the existing-file check makes a partial download sticky rather than
self-correcting.
3. Consideration: _failMsg puts subprocess stderr into a user-facing error
jf rt dl stderr can include the resolved URL, and depending on configuration
that can carry an access token. Worth deciding deliberately whether to pass it
through, truncate it, or scrub anything URL-shaped — not a blocker, just easy to
overlook until it lands in someone's CI log.
One thing I want to confirm rather than assume: a jf failure returns false
and deliberately does not fall back to HTTP, while jf being unusable returns
null and does. That reads intentional — falling back to unauthenticated HTTP
after an auth failure would just 401, or worse quietly fetch something else —
and the doc comment says so. Flagging only because the asymmetry is load-bearing
and easy to "simplify" away later.
2 participants
Description
For configured Yocto sdk_url that points to an Artifactory location, verifies jfrog CLI is available and uses it to download the SDK.
Type of Change