Avoid compressing cli files into zip for platform ext#1563
Merged
kaje94 merged 1 commit intorelease/bi-1.8.xfrom Feb 25, 2026
Merged
Avoid compressing cli files into zip for platform ext#1563kaje94 merged 1 commit intorelease/bi-1.8.xfrom
kaje94 merged 1 commit intorelease/bi-1.8.xfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request refactors the Choreo CLI download and installation process. The configuration declares expected output folders, version is bumped, the download script transitions from combined-zip to per-platform binary extraction with a new CLI_PLATFORMS mapping structure, and the CLI install module is simplified to validate pre-existing binaries rather than handling extraction. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js (4)
263-308: No timeout on HTTPS requests.
downloadFileandhttpsRequestdon't set socket/request timeouts. If a connection hangs (e.g., network interruption mid-download), the build script will block indefinitely. Since this runs in CI/CD pipelines, a stalled build can be costly.Example: add a timeout to downloadFile's request
const req = https.request(requestUrl, { headers: { 'User-Agent': 'Choreo-CLI-Downloader', 'Accept': 'application/octet-stream', ...getAuthHeaders() - } + }, + timeout: 120_000 // 2 minutes }, (res) => {And handle the timeout event:
req.on('timeout', () => { req.destroy(new Error('Request timed out')); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js` around lines 263 - 308, downloadFile currently never times out, so hanging HTTPS requests will block CI; update the request logic inside downloadFile (function downloadFile and its inner makeRequest) to set a request/socket timeout (e.g., a configurable timeoutMs constant or parameter), attach handlers for timeout events (req.on('timeout', ...) and/or req.on('socket', socket => socket.setTimeout(...))), and on timeout destroy the request (req.destroy(new Error('Request timed out'))), then call cleanupAndReject with that error so the file is closed/removed and the Promise rejects; ensure the same timeout handling is applied for redirected requests in makeRequest and that req.on('error', ...) still routes errors to cleanupAndReject.
104-179: Sync logic is thorough butcheckExistingCLIreturningfalsetriggers a full re-download of all platforms.When the partial-sync path (Lines 148-178) leaves some platforms still missing, returning
falsecausesdownloadAndExtractCLIto re-download all platforms — including those already present. Consider passing thestillMissinglist to the download function so it only fetches what's actually needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js` around lines 104 - 179, checkExistingCLI currently returns false when some platforms remain missing which forces downloadAndExtractCLI to re-download all platforms; modify checkExistingCLI to return the list of missing platforms (stillMissing) or an object { allPresent: boolean, missing: Platform[] } instead of a boolean, and update callers (e.g., downloadAndExtractCLI) to accept that missing list and only download those entries; update function references to checkExistingCLI and any call sites that expect a boolean to handle the new return shape and pass the missing list into downloadAndExtractCLI so only stillMissing platforms are fetched.
181-192: Old versions are cleaned up before verifying the new version is available.
cleanupOldFilesruns at Line 431 beforecheckExistingCLIat Line 434. If the current version's binaries aren't present and the subsequent download fails (network error, rate limit, missing release), the old working version has already been deleted. This leaves no fallback.This is likely acceptable for CI builds but worth being aware of for local developer workflows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js` around lines 181 - 192, cleanupOldFiles currently removes old versions from CLI_RESOURCES_DIR and CLI_CACHE_DIR before the script verifies the new release (checkExistingCLI), which can leave no fallback if the download fails; change the flow so old files are not deleted until the new binary is successfully verified—either (a) call checkExistingCLI or perform the download+verification to a temporary dir first, then on success atomically move/rename into place and only then call cleanupOldFiles, or (b) modify cleanupOldFiles to move existing entries to a backup location (or keep the latest working version) and remove backups only after successful installation; reference cleanupOldFiles, checkExistingCLI, CLI_RESOURCES_DIR and CLI_CACHE_DIR when making the change.
19-25: Platform matrix is missing 32-bit ARM variant.
cli-install.ts'sgetArchitecture()can return"arm"for 32-bit ARM systems, butCLI_PLATFORMShas no correspondinglinux-armentry. Users on 32-bit ARM Linux will get a "binary not found" error at runtime instead of an "unsupported architecture" error. If the Choreo CLI doesn't publish 32-bit ARM binaries, consider adding an explicit architecture check ingetArchitecture()to reject"arm"early with a clear error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js` around lines 19 - 25, CLI_PLATFORCTIONS lacks a linux-arm (32-bit ARM) entry while getArchitecture() can return "arm", causing a confusing "binary not found" error; update the code so getArchitecture() explicitly rejects the "arm" return value with a clear, deterministic error (e.g., throw or return a failure message stating 32-bit ARM is unsupported) or alternatively add a { assetSuffix: 'linux-arm', os: 'linux', arch: 'arm', ext: 'tar.gz', binary: 'choreo' } entry to CLI_PLATFORMS if 32-bit ARM binaries are actually published; reference getArchitecture() and CLI_PLATFORMS when making the change so runtime behavior is consistent and users see a helpful unsupported-architecture message instead of a missing-binary error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js`:
- Around line 390-397: The loop over CLI_PLATFORMS currently logs a warning and
continues when an asset is missing (asset = releaseData.assets?.find(...)),
which allows downloadAndExtractCLI to succeed with missing platform artifacts;
instead, make the check for assetName (using getAssetName) fail fast by throwing
an Error or exiting the process when asset is not found so the build stops and
the extension cannot be produced in a partially broken state. Update the block
that references CLI_PLATFORMS, getAssetName, and releaseData.assets to replace
the console.warn + continue behavior with a thrown error (or process.exit with
non-zero code) including the missing assetName and version to aid debugging.
- Around line 334-363: The unzip invocation in extractBinary is missing the {
stdio: 'inherit' } option so its output/errors are not forwarded like the tar
and PowerShell calls; update the execSync call that runs unzip (the branch
handling platform.ext === '.zip' on non-win32) to pass { stdio: 'inherit' }
(consistent with the tar and powershell calls) so failures and diagnostic output
from extracting archivePath to tmpExtractDir (used by findFile and then copied
to destDir/platform.binary) are visible.
---
Nitpick comments:
In
`@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js`:
- Around line 263-308: downloadFile currently never times out, so hanging HTTPS
requests will block CI; update the request logic inside downloadFile (function
downloadFile and its inner makeRequest) to set a request/socket timeout (e.g., a
configurable timeoutMs constant or parameter), attach handlers for timeout
events (req.on('timeout', ...) and/or req.on('socket', socket =>
socket.setTimeout(...))), and on timeout destroy the request (req.destroy(new
Error('Request timed out'))), then call cleanupAndReject with that error so the
file is closed/removed and the Promise rejects; ensure the same timeout handling
is applied for redirected requests in makeRequest and that req.on('error', ...)
still routes errors to cleanupAndReject.
- Around line 104-179: checkExistingCLI currently returns false when some
platforms remain missing which forces downloadAndExtractCLI to re-download all
platforms; modify checkExistingCLI to return the list of missing platforms
(stillMissing) or an object { allPresent: boolean, missing: Platform[] } instead
of a boolean, and update callers (e.g., downloadAndExtractCLI) to accept that
missing list and only download those entries; update function references to
checkExistingCLI and any call sites that expect a boolean to handle the new
return shape and pass the missing list into downloadAndExtractCLI so only
stillMissing platforms are fetched.
- Around line 181-192: cleanupOldFiles currently removes old versions from
CLI_RESOURCES_DIR and CLI_CACHE_DIR before the script verifies the new release
(checkExistingCLI), which can leave no fallback if the download fails; change
the flow so old files are not deleted until the new binary is successfully
verified—either (a) call checkExistingCLI or perform the download+verification
to a temporary dir first, then on success atomically move/rename into place and
only then call cleanupOldFiles, or (b) modify cleanupOldFiles to move existing
entries to a backup location (or keep the latest working version) and remove
backups only after successful installation; reference cleanupOldFiles,
checkExistingCLI, CLI_RESOURCES_DIR and CLI_CACHE_DIR when making the change.
- Around line 19-25: CLI_PLATFORCTIONS lacks a linux-arm (32-bit ARM) entry
while getArchitecture() can return "arm", causing a confusing "binary not found"
error; update the code so getArchitecture() explicitly rejects the "arm" return
value with a clear, deterministic error (e.g., throw or return a failure message
stating 32-bit ARM is unsupported) or alternatively add a { assetSuffix:
'linux-arm', os: 'linux', arch: 'arm', ext: 'tar.gz', binary: 'choreo' } entry
to CLI_PLATFORMS if 32-bit ARM binaries are actually published; reference
getArchitecture() and CLI_PLATFORMS when making the change so runtime behavior
is consistent and users see a helpful unsupported-architecture message instead
of a missing-binary error.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/wso2-platform/wso2-platform-extension/config/rush-project.jsonworkspaces/wso2-platform/wso2-platform-extension/package.jsonworkspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.jsworkspaces/wso2-platform/wso2-platform-extension/src/choreo-rpc/cli-install.ts
Comment on lines
+334
to
363
| function extractBinary(archivePath, platform, destDir) { | ||
| ensureDirectoryExists(destDir); | ||
| const tmpExtractDir = fs.mkdtempSync(path.join(os.tmpdir(), `choreo-extract-`)); | ||
|
|
||
| try { | ||
| if (platform.ext === '.tar.gz') { | ||
| execSync(`tar -xzf '${archivePath}' -C '${tmpExtractDir}'`, { stdio: 'inherit' }); | ||
| } else if (platform.ext === '.zip') { | ||
| if (os.platform() === 'win32') { | ||
| execSync(`powershell.exe -Command "Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpExtractDir}' -Force"`, { stdio: 'inherit' }); | ||
| } else { | ||
| execSync(`unzip -q '${archivePath}' -d '${tmpExtractDir}'`); | ||
| } | ||
| } | ||
|
|
||
| // Find the binary recursively (it may be inside a subdirectory) | ||
| const binaryPath = findFile(tmpExtractDir, platform.binary); | ||
| if (!binaryPath) { | ||
| throw new Error(`Binary '${platform.binary}' not found after extracting ${archivePath}`); | ||
| } | ||
|
|
||
| const destPath = path.join(destDir, platform.binary); | ||
| fs.copyFileSync(binaryPath, destPath); | ||
| if (platform.binary !== 'choreo.exe') { | ||
| fs.chmodSync(destPath, 0o755); | ||
| } | ||
| console.log(` ✓ Extracted ${platform.os}/${platform.arch} binary`); | ||
| } finally { | ||
| fs.rmSync(tmpExtractDir, { recursive: true, force: true }); | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent stdio option on unzip command.
Line 340 (tar) and Line 343 (powershell) both pass { stdio: 'inherit' }, but Line 345 (unzip) does not. This means extraction errors/output from unzip on macOS/Linux will be silently swallowed. execSync will still throw on a non-zero exit code, but diagnostic output won't be visible.
Proposed fix
- execSync(`unzip -q '${archivePath}' -d '${tmpExtractDir}'`);
+ execSync(`unzip -q '${archivePath}' -d '${tmpExtractDir}'`, { stdio: 'inherit' });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function extractBinary(archivePath, platform, destDir) { | |
| ensureDirectoryExists(destDir); | |
| const tmpExtractDir = fs.mkdtempSync(path.join(os.tmpdir(), `choreo-extract-`)); | |
| try { | |
| if (platform.ext === '.tar.gz') { | |
| execSync(`tar -xzf '${archivePath}' -C '${tmpExtractDir}'`, { stdio: 'inherit' }); | |
| } else if (platform.ext === '.zip') { | |
| if (os.platform() === 'win32') { | |
| execSync(`powershell.exe -Command "Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpExtractDir}' -Force"`, { stdio: 'inherit' }); | |
| } else { | |
| execSync(`unzip -q '${archivePath}' -d '${tmpExtractDir}'`); | |
| } | |
| } | |
| // Find the binary recursively (it may be inside a subdirectory) | |
| const binaryPath = findFile(tmpExtractDir, platform.binary); | |
| if (!binaryPath) { | |
| throw new Error(`Binary '${platform.binary}' not found after extracting ${archivePath}`); | |
| } | |
| const destPath = path.join(destDir, platform.binary); | |
| fs.copyFileSync(binaryPath, destPath); | |
| if (platform.binary !== 'choreo.exe') { | |
| fs.chmodSync(destPath, 0o755); | |
| } | |
| console.log(` ✓ Extracted ${platform.os}/${platform.arch} binary`); | |
| } finally { | |
| fs.rmSync(tmpExtractDir, { recursive: true, force: true }); | |
| } | |
| function extractBinary(archivePath, platform, destDir) { | |
| ensureDirectoryExists(destDir); | |
| const tmpExtractDir = fs.mkdtempSync(path.join(os.tmpdir(), `choreo-extract-`)); | |
| try { | |
| if (platform.ext === '.tar.gz') { | |
| execSync(`tar -xzf '${archivePath}' -C '${tmpExtractDir}'`, { stdio: 'inherit' }); | |
| } else if (platform.ext === '.zip') { | |
| if (os.platform() === 'win32') { | |
| execSync(`powershell.exe -Command "Expand-Archive -Path '${archivePath}' -DestinationPath '${tmpExtractDir}' -Force"`, { stdio: 'inherit' }); | |
| } else { | |
| execSync(`unzip -q '${archivePath}' -d '${tmpExtractDir}'`, { stdio: 'inherit' }); | |
| } | |
| } | |
| // Find the binary recursively (it may be inside a subdirectory) | |
| const binaryPath = findFile(tmpExtractDir, platform.binary); | |
| if (!binaryPath) { | |
| throw new Error(`Binary '${platform.binary}' not found after extracting ${archivePath}`); | |
| } | |
| const destPath = path.join(destDir, platform.binary); | |
| fs.copyFileSync(binaryPath, destPath); | |
| if (platform.binary !== 'choreo.exe') { | |
| fs.chmodSync(destPath, 0o755); | |
| } | |
| console.log(` ✓ Extracted ${platform.os}/${platform.arch} binary`); | |
| } finally { | |
| fs.rmSync(tmpExtractDir, { recursive: true, force: true }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js`
around lines 334 - 363, The unzip invocation in extractBinary is missing the {
stdio: 'inherit' } option so its output/errors are not forwarded like the tar
and PowerShell calls; update the execSync call that runs unzip (the branch
handling platform.ext === '.zip' on non-win32) to pass { stdio: 'inherit' }
(consistent with the tar and powershell calls) so failures and diagnostic output
from extracting archivePath to tmpExtractDir (used by findFile and then copied
to destDir/platform.binary) are visible.
Comment on lines
+390
to
+397
| for (const platform of CLI_PLATFORMS) { | ||
| const assetName = getAssetName(version, platform); | ||
| const asset = releaseData.assets?.find(a => a.name === assetName); | ||
|
|
||
| async function downloadAndCombineCLI(version) { | ||
| const tempDir = createTempDirectory(`choreo-cli-${version}-`); | ||
|
|
||
| try { | ||
| // Ensure both directories exist | ||
| ensureDirectoryExists(CLI_RESOURCES_DIR); | ||
| ensureDirectoryExists(CLI_CACHE_DIR); | ||
| if (!asset) { | ||
| console.warn(`Warning: Asset not found: ${assetName}`); | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing asset triggers continue — download succeeds partially without error.
If a GitHub release is missing an asset for one platform, the script logs a warning and continues. downloadAndExtractCLI will complete successfully, but the extension package will be broken for that platform. Consider failing the build instead of silently continuing, to avoid shipping an incomplete extension.
Proposed fix — fail fast on missing asset
if (!asset) {
- console.warn(`Warning: Asset not found: ${assetName}`);
- continue;
+ throw new Error(`Required asset not found in release: ${assetName}. Available assets: ${(releaseData.assets || []).map(a => a.name).join(', ')}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const platform of CLI_PLATFORMS) { | |
| const assetName = getAssetName(version, platform); | |
| const asset = releaseData.assets?.find(a => a.name === assetName); | |
| async function downloadAndCombineCLI(version) { | |
| const tempDir = createTempDirectory(`choreo-cli-${version}-`); | |
| try { | |
| // Ensure both directories exist | |
| ensureDirectoryExists(CLI_RESOURCES_DIR); | |
| ensureDirectoryExists(CLI_CACHE_DIR); | |
| if (!asset) { | |
| console.warn(`Warning: Asset not found: ${assetName}`); | |
| continue; | |
| } | |
| for (const platform of CLI_PLATFORMS) { | |
| const assetName = getAssetName(version, platform); | |
| const asset = releaseData.assets?.find(a => a.name === assetName); | |
| if (!asset) { | |
| throw new Error(`Required asset not found in release: ${assetName}. Available assets: ${(releaseData.assets || []).map(a => a.name).join(', ')}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@workspaces/wso2-platform/wso2-platform-extension/scripts/download-choreo-cli.js`
around lines 390 - 397, The loop over CLI_PLATFORMS currently logs a warning and
continues when an asset is missing (asset = releaseData.assets?.find(...)),
which allows downloadAndExtractCLI to succeed with missing platform artifacts;
instead, make the check for assetName (using getAssetName) fail fast by throwing
an Error or exiting the process when asset is not found so the build stops and
the extension cannot be produced in a partially broken state. Update the block
that references CLI_PLATFORMS, getAssetName, and releaseData.assets to replace
the console.warn + continue behavior with a thrown error (or process.exit with
non-zero code) including the missing assetName and version to aid debugging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
$subject
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
Chores
Refactor