Skip to content
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

feat: workspaces::sandbox() can connect to user provided sandbox node #220

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

ChaoticTempest
Copy link
Member

This adds a Builder to the return type of workspaces::sandbox() which is another async builder so that users can parameterize what RPC url they want to point to if they have a different RPC node pointing to that network. For this PR, only sandbox was done, and will add testnet and mainnet after since those are pretty trivial in comparison.

Sandbox now basically has the ability to connect to a user's own locally spawned sandbox node.

Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also be nice to add a test for this

workspaces/src/network/builder.rs Outdated Show resolved Hide resolved
workspaces/src/network/builder.rs Outdated Show resolved Hide resolved
examples/manually-spawned-sandbox.md Outdated Show resolved Hide resolved
workspaces/src/network/server.rs Outdated Show resolved Hide resolved
@ChaoticTempest
Copy link
Member Author

Rebased this with latest changes on workspaces. @itegulov let me know how it looks when you get the chance

@ChaoticTempest ChaoticTempest removed the request for review from austinabell January 14, 2023 02:16
Comment on lines +156 to +157
"Cleaning up sandbox: port={:?}, pid={}",
rpc_port,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For messages like these, isn't just a port lossy? Especially since custom sandbox Urls could technically have no port. So this would just be None.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, this was old messaging that I forgot and didn't notice to change. Thanks for pointing this out

workspaces/src/network/server.rs Show resolved Hide resolved
@@ -111,22 +148,20 @@ impl Drop for SandboxServer {
return;
}

let rpc_port = self.rpc_port();
let child = self.process.as_mut().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would panic for custom sandbox urls.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For custom sandbox URLs, there's no process spawned, so child would be None here and early return in the above:

        if self.process.is_none() {
            return;
        }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that code path was collapsed in the diff. Looks good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanna try this at the top instead?

let child = match self.process.take() {
    Some(child) => child,
    None => return,
};

@nabeel99
Copy link

hey @ChaoticTempest , as I stated on discord , I am trying to integrate workspaces local/sandbox tests with the kurtosis local environment, would it be possible to use this branch to connect to it ? if not what roadblock would prevent me from doing it ?

@itegulov
Copy link
Contributor

@miraclx any objections to merging this?

Copy link
Contributor

@miraclx miraclx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None at all. LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants