-
Notifications
You must be signed in to change notification settings - Fork 1
Add roxyd binary entrypoint and QUIC/mTLS config (#528) #531
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
sae-bom
left a comment
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.
-
The claim that “all fields are optional to ensure legacy configurations remain unaffected” is incorrect. The legacy
roxybinary does not use these structs at all. Therefore, these fields should be required rather than wrapped inOption. -
The custom
parse_args()implementation is fragile, C-style, and difficult to test. I recommend using theclapcrate instead, which is the standard approach for building Rust CLIs. -
I also recommend using the
configcrate, as it supports environment variable overrides, merging multiple configuration sources, and provides better validation. -
Adding
pub mod configtosrc/lib.rsunnecessarily pollutes the library’s public API withroxyd-specific types that the legacyroxybinary never uses. Please restructure this as a binary-specific module instead, for example:src/ ├── lib.rs (no pub mod config) ├── bin/ │ └── roxyd/ │ ├── main.rs (mod config; use config::...) │ └── config.rs └── main.rs -
Also, when importing external crates, our team convention is to specify versions only up to the minor level (e.g., 2.5) and omit the patch version.
-
Please also update the related documentation accordingly.
a231654 to
81be891
Compare
|
I reviewed the feedback and have implemented the requested changes. All changes are committed and pushed. What I changed and why
Notes on behavior
Thanks for the clear feedback — I addressed each point and pushed the updates. If you want, I can:
|
|
Please fix the CI failure caused by |
516f5e5 to
ef06979
Compare
|
Thanks — I looked into the CI failure and found the cause. There were two binary targets named roxyd: src/bin/roxyd.rs (old) and src/bin/roxyd/main.rs (new), which produced a "duplicate binary name" error. I removed the outdated src/bin/roxyd.rs, committed, and pushed the change. That clears the duplicate target and should resolve the CI error. Thanks for the report — everything has been addressed, but let me know if CI still complains. |
zmrdltl
left a comment
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.
- Commit messages would be clearer if they state what changed and why. Please update them to meaningful summaries that follow the project conventions.
- Several comments repeat the code behavior verbatim, which makes the file verbose. Please remove unnecessary comments and keep them focused on intent or non‑obvious rationale.
Introduce a new binary entrypoint for the planned QUIC/mTLS communication path with the Manager. This is scaffolding only; no protocol handlers are implemented yet. - Add src/bin/roxyd/main.rs with CLI parsing, config loading, and tracing - Add src/bin/roxyd/config.rs with RoxydConfig, QuicConfig, MtlsConfig - Update Cargo.toml with config and toml dependencies - Update README.md and CHANGELOG.md Closes #528
1558106 to
8564388
Compare
|
Thanks for the review — I analyzed each point and found they were already handled on the branch I force-pushed, so no additional changes are needed. Summary of what I addressed:
All tests pass and clippy reports no issues. Thanks for the suggestions — they helped validate the changes. If you'd like, I can point to the specific commit or lines where each change lives. |
|
roxyd currently keeps CLI arguments ( roxyd (current)
Other projects observed patterns
Personally, I think keeping |
|
Whether to use an Args struct or parse manually seems like an implementation detail. I think the core issue is deciding which settings belong in CLI arguments versus the TOML configuration file. What are your thoughts on the criteria for separating them? Does the current PR's code reflect your suggestion, or are you planning to adjust it? |
|
I want to maximize consistency, so I’m suggesting a direction for octoaide. CLI
TOML
One more question: we usually use |
This PR adds a new roxyd binary entrypoint and a minimal, non-invasive configuration skeleton for QUIC/mTLS connectivity as the first step toward the Manager <-> roxyd design discussed in #519.
What changed
Design notes / constraints
How to try
Files of interest
CI / Acceptance
Closes #528