feat: implement wildcard resolution into the action - #93
Conversation
bun install is rqeuired for patches
bun install is rqeuired for patches
|
Thank you for this, but I think we should fix the endpoint instead of changing the code in the setup action. It’s best to keep the action as small as possible. |
I don't want to be rude but I've been requesting a fix in our private staff Discord channel for a while with no response. People are asking for it in the issue #37 without any response from the core team. Since this issue is affecting people, I decided to finally resolve this issue even though I'm on vacation. You just closed this pull request and only said that it's better to fix the API, but when can we expect this issue to be resolved? I transferred this action to the oven-sh organisation because you asked for it. I'm happy to have been able to do so, but the API problems are making it harder for me to maintain setup-bun action. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/action.ts (2)
175-175: Critical: Missing authentication parameter in downloadTool call.The
downloadToolcall does not passdownloadMeta.auth, even though theDownloadMetatype includes an optionalauthfield. This means authenticated downloads (e.g., for GitHub API with rate limits or private artifacts) will fail or be subject to stricter rate limits.🔎 Proposed fix
- const zipPath = addExtension(await downloadTool(downloadMeta.url), ".zip"); + const zipPath = addExtension( + await downloadTool(downloadMeta.url, undefined, downloadMeta.auth), + ".zip" + );
32-32: Maketokenoptional.The
tokenfield is defined as required, but the code already handles missing tokens gracefully using conditional checks (see lines 43-45, 68-70, 95-97 insrc/download-url.ts). Since public downloads don't require authentication and theauthfield inDownloadMetais already optional, the type should reflect this. GitHub Actions inputs are typically optional unless explicitly required.🔎 Suggested fix
- token: string; + token?: string;
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
dist/setup/index.jsis excluded by!**/dist/**
📒 Files selected for processing (1)
src/action.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/action.ts (2)
src/download-url.ts (2)
getDownloadMeta(10-23)DownloadMeta(5-8)src/utils.ts (2)
retry(6-19)addExtension(36-43)
🔇 Additional comments (3)
src/action.ts (3)
54-55: LGTM!The download metadata retrieval and URL extraction are correctly implemented.
170-173: LGTM!The updated signature correctly accepts
DownloadMetainstead of a raw URL, allowing both URL and authentication to be passed together.
147-168: LGTM!The version matching logic correctly handles:
- Rejecting matches for non-pinned versions (latest/canary/action) to ensure fresh downloads
- Normalizing version strings to handle 'v' prefixes
- Extracting version from revision strings
| revision = await retry( | ||
| async () => await downloadBun(downloadMeta, bunPath), | ||
| 3, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Retry logic correctly integrated with new signature.
The downloadBun call properly passes downloadMeta and the retry wrapper with 3 attempts is appropriate for network operations.
🔎 Optional: simplify redundant await
The await inside the arrow function is redundant since the arrow function is already async and the result is awaited by retry. You can simplify to:
- revision = await retry(
- async () => await downloadBun(downloadMeta, bunPath),
- 3,
- );
+ revision = await retry(
+ () => downloadBun(downloadMeta, bunPath),
+ 3,
+ );📝 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.
| revision = await retry( | |
| async () => await downloadBun(downloadMeta, bunPath), | |
| 3, | |
| ); | |
| revision = await retry( | |
| () => downloadBun(downloadMeta, bunPath), | |
| 3, | |
| ); |
🤖 Prompt for AI Agents
In src/action.ts around lines 114 to 117, the arrow function passed to retry
uses a redundant inner await (async () => await downloadBun(...)); remove the
unnecessary await by returning the promise directly — e.g. use async () =>
downloadBun(downloadMeta, bunPath) or simply () => downloadBun(downloadMeta,
bunPath) so retry receives the promise-returning function without the extra
await.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/test.yml (1)
61-65: Remove stale commented-out code.These commented lines are now redundant since the same version specs have been re-enabled at lines 57-60. The reference to issue #37 is useful context but could be preserved as a simpler comment if desired.
🔎 Proposed cleanup
- "1.x" - "1" - "> 1.0.0" - "< 2" - # https://github.com/oven-sh/setup-bun/issues/37 - # - "1.x" - # - "1" - # - "> 1.0.0" - # - "< 2" # Disable <sha> support for now. This is because Github Artifacts
♻️ Duplicate comments (1)
src/download-url.ts (1)
77-79: Consider strict equality for boolean checks.Using
avx2 == false(loose equality) works correctly here becauseundefined == falseevaluates tofalse. However, strict equality (===) is more idiomatic in TypeScript and makes the intent clearer.🔎 Proposed change
const name = `bun-${os ?? getPlatform()}-${arch ?? getArchitecture()}${ - avx2 == false ? "-baseline" : "" + avx2 === false ? "-baseline" : "" }${profile ? "-profile" : ""}`;
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
dist/setup/index.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
.github/actions/compare-bun-version/action.yml.github/workflows/test.ymlsrc/download-url.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/download-url.ts (2)
src/action.ts (1)
Input(23-33)src/utils.ts (3)
request(21-34)getPlatform(45-50)getArchitecture(52-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: setup-bun (windows-latest, canary)
🔇 Additional comments (5)
.github/actions/compare-bun-version/action.yml (1)
28-34: LGTM on version comparison function.Using
sort -Vfor semantic version comparison is a solid approach that handles most version formats correctly..github/workflows/test.yml (1)
85-88: Good addition for version verification.This step ensures the installed Bun version actually satisfies the requested spec, providing valuable regression coverage for the wildcard resolution feature.
src/download-url.ts (3)
1-8: LGTM on module structure.The imports align with the exported utilities from
./utilsand theInputtype from./action. TheDownloadMetainterface provides a clean abstraction for the download URL and optional authentication.
10-23: Clean dispatcher logic.The function correctly prioritizes custom URL, then detects SHA-based versions via the 40-character hex pattern, and falls back to semver resolution. The flow is clear and efficient.
110-126: Tag resolution logic handles edge cases well.The logic correctly handles:
- Exact tag matches (including "canary")
- "latest" or undefined version → latest valid tag
- Semver constraints via
satisfies()- Appropriate error when no match is found
The
validate()check at line 124 ensures non-semver tags like "canary" aren't incorrectly prefixed withbun-v.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils.ts (1)
77-93: Unreachable return statement infinallyblock.The
return outputin thefinallyblock (line 91) is unreachable whenoutputis truthy because theif (!output)block at lines 81-84 already returnsundefinedvia an implicit return when warning is called. Thefinallyblock always executes, but the return at line 91 only triggers ifoutputis truthy—which means the function would have already completed the try block successfully. This logic is confusing and thefinallyreturn may cause unexpected behavior.🔎 Proposed fix for clearer control flow
export function readVersionFromFile(file: string): string | undefined { const cwd = process.env.GITHUB_WORKSPACE; if (!cwd) { return; } if (!file) { return; } debug(`Reading version from ${file}`); const path = resolve(cwd, file); const base = basename(file); if (!existsSync(path)) { warning(`File ${path} not found`); return; } const reader = FILE_VERSION_READERS[base] ?? (() => undefined); - let output: string | undefined; try { - output = reader(readFileSync(path, "utf8"))?.trim(); + const output = reader(readFileSync(path, "utf8"))?.trim(); if (!output) { warning(`Failed to read version from ${file}`); - return; + return undefined; } + + info(`Obtained version ${output} from ${file}`); + return output; } catch (error) { const { message } = error as Error; warning(`Failed to read ${file}: ${message}`); - } finally { - if (output) { - info(`Obtained version ${output} from ${file}`); - return output; - } + return undefined; } }src/action.ts (1)
166-183: Auth token not passed todownloadToolfor SHA-based downloads.
downloadBunreceivesdownloadMetawhich includes an optionalauthfield (set for SHA-based artifact downloads), but line 171 only passesdownloadMeta.urltodownloadTool. The@actions/tool-cachedownloadToolfunction accepts an optionalauthparameter as the third argument, which is needed for authenticated GitHub artifact downloads.🔎 Proposed fix
async function downloadBun( downloadMeta: DownloadMeta, bunPath: string, ): Promise<string | undefined> { // Workaround for https://github.com/oven-sh/setup-bun/issues/79 and https://github.com/actions/toolkit/issues/1179 - const zipPath = addExtension(await downloadTool(downloadMeta.url), ".zip"); + const zipPath = addExtension( + await downloadTool(downloadMeta.url, undefined, downloadMeta.auth), + ".zip", + ); const extractedZipPath = await extractZip(zipPath);
♻️ Duplicate comments (1)
src/action.ts (1)
32-32: Consider makingtokenoptional.The
tokenfield is marked as required, but the download flow insrc/download-url.tsalready handles missing tokens gracefully by conditionally omitting the Authorization header. Making this optional would align the type with the actual runtime behavior where unauthenticated requests are valid (though rate-limited).🔎 Proposed fix
export type Input = { customUrl?: string; version?: string; os?: string; arch?: string; avx2?: boolean; profile?: boolean; registries?: Registry[]; noCache?: boolean; - token: string; + token?: string; };
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
dist/setup/index.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
.github/actions/compare-bun-version/action.ymlsrc/action.tssrc/download-url.tssrc/utils.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/action.ts (2)
src/download-url.ts (2)
getDownloadMeta(10-23)DownloadMeta(5-8)src/utils.ts (1)
addExtension(21-28)
src/download-url.ts (2)
src/action.ts (2)
options(50-141)Input(23-33)src/utils.ts (3)
request(6-19)getPlatform(30-35)getArchitecture(37-42)
🔇 Additional comments (11)
src/utils.ts (2)
6-18: LGTM!The
requesthelper properly handles non-OK responses by reading the body text and including it in the error message, providing actionable debugging information.
30-42: Architecture mapping is correct.Bun's release artifacts confirm the naming convention:
aarch64for ARM64 andx64for x64 (e.g.,bun-linux-x64.zip,bun-darwin-aarch64.zip). ThegetArchitecture()function correctly mapsarm64→aarch64and returnsx64unchanged, matching Bun's actual release naming.src/action.ts (1)
54-55: LGTM!The refactored flow correctly obtains download metadata via
getDownloadMetaand extracts the URL for cache key generation and logging purposes.src/download-url.ts (3)
10-23: LGTM!The routing logic correctly handles the three cases: custom URL passthrough, SHA-based artifact lookup, and semver resolution.
34-90: SHA-based download flow is well-structured.The paginated search with proper termination conditions (empty results, found run), error handling for missing runs/artifacts, and conditional auth headers are all correctly implemented per previous review feedback.
101-127: Canary version handling is correct and working as intended.The code properly handles the "canary" version: when a user requests
version: "canary", the tag is found at line 110 before the semver validation filter is applied. Sincevalidate("canary")returns false, the tag remains unprefixed, resulting in the correct URL format (canary/bun-linux-x64.zip). Verification confirms GitHub recognizes and serves this URL format correctly..github/actions/compare-bun-version/action.yml (5)
12-17: LGTM!Good use of
tr -d '\r\n'to handle cross-platform line endings, and graceful fallback for--revisionon older Bun versions that may not support it.
30-34:sort -Vmay not be available on all runners.The
sort -V(version sort) flag is a GNU coreutils extension. While it's available on Ubuntu runners, macOS runners use BSD sort which also supports-V, but some minimal environments may not. This is likely fine for GitHub-hosted runners but worth noting.
64-72: Single-segment version without operator defaults to prefix match.When
opis==(default) andversion_parthas no dots (e.g.,1), the condition at line 64 treats it as a wildcard. This meansbun-version: "1"will match any1.x.yversion, which aligns with semver wildcard semantics but differs from exact match behavior. This is likely intentional but worth documenting in the action's description.
26-86: Well-structured version comparison logic.The script handles multiple version specification formats (latest, canary, exact versions, ranges, wildcards) with clear error messages. The use of
set -euo pipefailensures failures are properly propagated.
61-61: Wildcard replacement only removes first.xoccurrence.The pattern
${version_part//.x/}uses single/which only replaces the first occurrence. A spec like1.x.xwould become1.xinstead of1. Use${version_part//.x/}with double slash for global replacement:${version_part//.x}(remove all.x).🔎 Proposed fix
- version_base="${version_part//.x/}" + version_base="${version_part//.x}"Note: The syntax
${var//pattern}removes all occurrences ofpatternfromvar.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
.github/workflows/test.yml:65
- These commented-out test cases are now duplicates of the active test cases on lines 57-60. The comments indicating they were disabled due to issue #37 are now obsolete since this PR implements the wildcard resolution feature. These duplicate commented lines should be removed.
# https://github.com/oven-sh/setup-bun/issues/37
# - "1.x"
# - "1"
# - "> 1.0.0"
# - "< 2"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
resolves #37, #126
brings back #124
https://bun.sh/downloadis not used anymore