-
Notifications
You must be signed in to change notification settings - Fork 925
fix: align behavior and API surface between the JS and Python SDKs #1411
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
Open
mishushakov
wants to merge
8
commits into
main
Choose a base branch
from
mishushakov/python-js-sdk-alignment
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
7343204
fix: align behavior between the JS and Python SDKs
mishushakov 5161759
Merge remote-tracking branch 'origin/main' into mishushakov/python-js…
mishushakov 79b1d13
chore: drop pause bullet from changeset (already shipped on main)
mishushakov f034315
fix: skip get_metrics in debug mode at the instance level (JS + Python)
mishushakov 22ab7c8
test: prove requestTimeoutMs 0 disables the timeout through the confi…
mishushakov a7646d5
refactor(js-sdk): remove getFullInfo, inline into getInfo
mishushakov ee2230b
fix: skip kill/get_metrics in debug on both instance and class methods
mishushakov 5389f4b
test: add git validation unit test (JS) and stdin bytes cases (JS + P…
mishushakov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| --- | ||
| "e2b": patch | ||
| "@e2b/python-sdk": patch | ||
| "@e2b/cli": patch | ||
| --- | ||
|
|
||
| fix: align behavior between the JS and Python SDKs | ||
|
|
||
| Python SDK: | ||
|
|
||
| - `commands.send_stdin` and `CommandHandle.send_stdin` now accept `bytes` in addition to `str`, and the handle's `send_stdin` / `close_stdin` now accept a `request_timeout`. | ||
| - `git.reset` now accepts a typed `GitResetMode` and its validation error matches the JS SDK wording/ordering. `GitResetMode` is now exported. | ||
| - `sandbox_url` is now propagated through `get_api_params`. | ||
| - `Template.from_image()` now raises when only one of `username` / `password` is provided. | ||
|
|
||
| JS SDK: | ||
|
|
||
| - `Sandbox.getInfo()` now includes `sandboxDomain`; the `getFullInfo` method was removed (use `getInfo`), matching the Python SDK's single `get_info`. | ||
| - `Sandbox.getMetrics()` now returns `[]` in debug mode, matching the Python SDK. The debug short-circuit for `getMetrics` / `kill` is implemented on both the instance and static methods, so it applies consistently whether called as `Sandbox.kill(sandboxId)` or `sandbox.kill()`. | ||
| - `Template.fromImage()` now requires both `username` and `password` when registry credentials are provided. | ||
| - `Template.getBuildStatus()` now defaults `logsOffset` to `0`. | ||
| - `requestTimeoutMs: 0` now explicitly disables the request timeout. |
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import type { PathLike } from 'node:fs' | ||
| import { ApiClient } from '../api' | ||
| import { ConnectionConfig, ConnectionOpts } from '../connectionConfig' | ||
| import { BuildError } from '../errors' | ||
| import { BuildError, InvalidArgumentError } from '../errors' | ||
| import { runtime } from '../utils' | ||
| import { | ||
| assignTags, | ||
|
|
@@ -270,7 +270,7 @@ export class TemplateBase | |
| { | ||
| templateID: data.templateId, | ||
| buildID: data.buildId, | ||
| logsOffset: options?.logsOffset, | ||
| logsOffset: options?.logsOffset ?? 0, | ||
| }, | ||
| config.getSignal(undefined, options?.signal) | ||
| ) | ||
|
|
@@ -452,6 +452,13 @@ export class TemplateBase | |
|
|
||
| // Set the registry config if provided | ||
| if (credentials) { | ||
| if (!credentials.username || !credentials.password) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably should do credential check before doing the work above |
||
| throw new InvalidArgumentError( | ||
| 'Both username and password are required when providing registry credentials', | ||
| getCallerFrame(STACK_TRACE_DEPTH - 1) | ||
| ) | ||
| } | ||
|
|
||
| this.registryConfig = { | ||
| type: 'registry', | ||
| username: credentials.username, | ||
|
|
||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { test, expect } from 'vitest' | ||
|
|
||
| import { Git } from '../../../src/sandbox/git' | ||
| import type { Commands } from '../../../src/sandbox/commands' | ||
| import { InvalidArgumentError } from '../../../src/errors' | ||
|
|
||
| // Stub command runner that fails if a git command is actually executed — | ||
| // validation must throw before reaching it. | ||
| const failingCommands = { | ||
| run: () => { | ||
| throw new Error('commands.run should not be called') | ||
| }, | ||
| } as unknown as Commands | ||
|
|
||
| test('git.reset throws InvalidArgumentError on an invalid mode', async () => { | ||
| const git = new Git(failingCommands) | ||
| await expect( | ||
| // @ts-expect-error - testing runtime validation with an invalid mode | ||
| git.reset('/repo', { mode: 'bogus' }) | ||
| ).rejects.toThrow(InvalidArgumentError) | ||
| }) | ||
|
|
||
| test('git.reset accepts a valid mode', async () => { | ||
| const git = new Git(failingCommands) | ||
| // A valid mode must pass validation and reach the (stubbed) command runner. | ||
| await expect(git.reset('/repo', { mode: 'hard' })).rejects.toThrow( | ||
| 'commands.run should not be called' | ||
| ) | ||
| }) | ||
|
|
||
| test('git.remoteAdd throws InvalidArgumentError when name or url is missing', async () => { | ||
| const git = new Git(failingCommands) | ||
| await expect( | ||
| git.remoteAdd('/repo', '', 'https://example.com') | ||
| ).rejects.toThrow(InvalidArgumentError) | ||
| await expect(git.remoteAdd('/repo', 'origin', '')).rejects.toThrow( | ||
| InvalidArgumentError | ||
| ) | ||
| }) | ||
|
|
||
| test('git.remoteGet throws InvalidArgumentError when name is missing', async () => { | ||
| const git = new Git(failingCommands) | ||
| await expect(git.remoteGet('/repo', '')).rejects.toThrow(InvalidArgumentError) | ||
| }) |
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
maybe a reason to make major instead of minor but not end of world