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(windows): add subzone worker subprocess crate from abandoned IPC branch #3414

Closed
wants to merge 93 commits into from

Conversation

ReactorScram
Copy link
Collaborator

I wasn't sure if we should try to preserve the Git history at all, so I just stuck it in a folder in the same repo.

This branch is identical to main but has the crate. The subzone branch has a version of the Windows client that builds on top of subzone.

… and make sure normal execution doesn't depend on debug subcommands
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@ReactorScram ReactorScram self-assigned this Jan 26, 2024
Copy link

vercel bot commented Jan 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
firezone ⬜️ Ignored (Inspect) Visit Preview Jan 29, 2024 4:34pm

@ReactorScram ReactorScram marked this pull request as ready for review January 26, 2024 21:52
Copy link

github-actions bot commented Jan 26, 2024

Terraform Cloud Plan Output

Plan: 12 to add, 11 to change, 12 to destroy.

Terraform Cloud Plan

Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

Nice work on polishing this up! Do you think this crate is useful on its own (outside Firezone)? If so we could stick in a new repository and probably give it a more descriptive name for publishing on crates.io (though I do like subzone).

If not, then we could merge it for use with the pre-firezone-connection Windows client in case we need to stick around on webrtc-rs for longer than expected (though I foresee this possibility to be low)

@ReactorScram
Copy link
Collaborator Author

It would be more useful on its own, I agree we probably won't need it. If we get stuck on webrtc-rs we can just use this crate as a dependency

Copy link
Member

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Code LGTM!

The crate is currently neither tested in CI nor is it part of the workspace, meaning this code may bit-rot over time. At the same time, I don't really want to fix code that we don't actually use :)

My vote would go down for moving this into a separate repo that has a very basic Windows-only CI, i.e. fmt, clippy + test. That way we can evolve it if needed but we don't have to actively maintain it (dependency updates breaking stuff etc).

@thomaseizinger
Copy link
Member

thomaseizinger commented Jan 29, 2024

If we get stuck on webrtc-rs

I'll do what is within my powers to avoid this!

@jamilbk
Copy link
Member

jamilbk commented Jan 29, 2024

My vote would go down for moving this into a separate repo

Ah yeah, that sgtm. We can always choose to publish later on crates.io but not the other way around. @ReactorScram made you admin on https://github.com/firezone/subzone so you should be able to stick it there.

@ReactorScram
Copy link
Collaborator Author

Okay I'll move the code there, link this PR for the Git history, and then close this

@ReactorScram
Copy link
Collaborator Author

It's up and I copied some of the clippy / fmt / test CI from the main Firezone repo https://github.com/firezone/subzone

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.

None yet

3 participants