-
Notifications
You must be signed in to change notification settings - Fork 232
feat: publish passkety-bundler #2500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new GitHub Actions workflow for publishing the Passkey Bundler, improves Docker image compatibility, and enhances configuration handling. The changes are well-structured and clearly described. My review includes a minor simplification for the boolean environment variable parsing and suggestions to improve test hygiene by using temporary directories for database files in the new tests, ensuring they are isolated and leave no artifacts.
| test("testnet mode defaults authRequired=true and requires API keys", () => { | ||
| const entryPoint = "0x" + "11".repeat(20) | ||
| const privateKey = "0x" + "22".repeat(32) | ||
|
|
||
| withBundlerEnv( | ||
| { | ||
| BUNDLER_MODE: "testnet", | ||
| ENTRY_POINT: entryPoint, | ||
| BUNDLER_PRIVATE_KEY: privateKey, | ||
| DB_URL: "sqlite:./data/bundler.sqlite", | ||
| }, | ||
| () => { | ||
| assert.throws( | ||
| () => loadConfig(), | ||
| /Testnet mode requires API keys \(set BUNDLER_API_KEYS\) or disable authRequired/, | ||
| ) | ||
| }, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test writes to a hardcoded database path (sqlite:./data/bundler.sqlite), which can cause side effects and leave artifacts after test runs. To ensure test isolation and cleanliness, it's best practice to use a temporary directory for such files and clean it up afterwards, similar to how other tests in this file are structured.
test("testnet mode defaults authRequired=true and requires API keys", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "passkey-bundler-"))
try {
const entryPoint = "0x" + "11".repeat(20)
const privateKey = "0x" + "22".repeat(32)
withBundlerEnv(
{
BUNDLER_MODE: "testnet",
ENTRY_POINT: entryPoint,
BUNDLER_PRIVATE_KEY: privateKey,
DB_URL: `sqlite:${path.join(dir, "bundler.sqlite")}`,
},
() => {
assert.throws(
() => loadConfig(),
/Testnet mode requires API keys \(set BUNDLER_API_KEYS\) or disable authRequired/,
)
},
)
} finally {
fs.rmSync(dir, { recursive: true, force: true })
}
})| test("testnet mode respects BUNDLER_REQUIRE_AUTH=false", () => { | ||
| const entryPoint = "0x" + "11".repeat(20) | ||
| const privateKey = "0x" + "22".repeat(32) | ||
|
|
||
| const config = withBundlerEnv( | ||
| { | ||
| BUNDLER_MODE: "testnet", | ||
| ENTRY_POINT: entryPoint, | ||
| BUNDLER_PRIVATE_KEY: privateKey, | ||
| DB_URL: "sqlite:./data/bundler.sqlite", | ||
| BUNDLER_REQUIRE_AUTH: "false", | ||
| }, | ||
| () => loadConfig(), | ||
| ) | ||
|
|
||
| assert.equal(config.authRequired, false) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous test, this test writes to a hardcoded database path. This should be updated to use a temporary directory to ensure test isolation and prevent leaving artifacts.
test("testnet mode respects BUNDLER_REQUIRE_AUTH=false", () => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "passkey-bundler-"))
try {
const entryPoint = "0x" + "11".repeat(20)
const privateKey = "0x" + "22".repeat(32)
const config = withBundlerEnv(
{
BUNDLER_MODE: "testnet",
ENTRY_POINT: entryPoint,
BUNDLER_PRIVATE_KEY: privateKey,
DB_URL: `sqlite:${path.join(dir, "bundler.sqlite")}`,
BUNDLER_REQUIRE_AUTH: "false",
},
() => loadConfig(),
)
assert.equal(config.authRequired, false)
} finally {
fs.rmSync(dir, { recursive: true, force: true })
}
})| const toBool = (value?: string) => { | ||
| if (value === undefined) return undefined | ||
| const normalized = value.trim().toLowerCase() | ||
| if (!normalized) return undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request introduces a new GitHub Actions workflow for building and publishing the Passkey Bundler Docker image, updates the Docker base image for improved compatibility, enhances environment variable parsing, and adds new configuration-related tests. The changes improve the CI/CD pipeline, increase Docker image compatibility, and strengthen configuration validation.
CI/CD and Docker workflow improvements:
.github/workflows/passkey-bundler-docker.ymlto build, push, and publish multi-architecture Docker images for Passkey Bundler, including logic to detect relevant changes and handle release tags..github/workflows/release.ymlto ignorepasskey-bundler/v*tags, preventing duplicate releases for Docker-specific tags.Docker image compatibility:
passkey-bundler/Dockerfilefromnode:20-alpinetonode:20-bookworm-slimfor both build and runtime stages, improving compatibility with native dependencies.Configuration handling and testing:
passkey-bundler/src/config.tsto support more values (e.g., "y", "on", "n", "off") and handle whitespace/empty strings robustly.passkey-bundler/test/basic.test.tsto verify configuration loading, especially for testnet mode and authentication requirements. [1] [2]