-
Notifications
You must be signed in to change notification settings - Fork 15
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
janus_cli
: add DAP message decoder
#450
Conversation
I felt a certain tension while working on this PR: |
|
||
let mut binary_message = Cursor::new(message_buf.as_slice()); | ||
|
||
let decoded = match media_type { |
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.
I opted to only support the top-level messages that go on the wire as opposed to every single struct with a prio::codec::Decode
implementation, mainly so this match wouldn't get too sprawling and huge.
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.
LGTM, modulo updating the CLI reftests
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.
I felt a certain tension while working on this PR:
janus_cli
ought to eventually become a task provisioning, control plane type service that runs in cluster, and so it behaves in a lot of ways like otherjanus
server binaries, particularly in how it handles options and a config file.
I think I disagree with the direction stated here -- janus_cli
is a CLI tool, intended to be used manually by a human operator. Automated "control-plane" work would ideally be done by a separate job. (of course, that's the ideal/aspiration--I've seen plenty of times where "control plane work" actually was done by invoking some CLI tool with a cron job or the like--just trying to define how I see the Janus CLI's purpose for use when deciding how to grow it in the future)
My conception of how the configs would work is that you'd keep one config per environment around on your workstation, and refer to them as needed -- they won't change much/ever (barring backwards-incompatible changes to our config formats, of course), so they're effectively static config data.
Oh! I think you meant that |
Yes, you're right, we will want a tool that can talk to the control plane, and |
d25240a
to
4a05e16
Compare
Downgrading to draft while I re-work the argument handing here and in #448 |
4a05e16
to
8f51d7d
Compare
`janus_cli decode-dap-message` can read binary encoded DAP wire messages from either stdin or a file and then decode them as a specified message type, printing the `std::fmt::Debug` representation of the decoded message to stdout. `janus_cli` uses some of the scaffolding from the `janus` server binaries for option parsing, and so it used to expect to be provided a configuration file with logging and particularly datbase config. That isn't needed for the `decode-dap-message` command, so we lower the `CommonBinaryOptions` from `janus_cli::Options` down into those variants of `janus_cli::Command` that actually need those flags. Resolves #441
8f51d7d
to
d2f0e96
Compare
janus_cli decode-dap-message
can read binary encoded DAP wire messagesfrom either stdin or a file and then decode them as a specified message
type, printing the
std::fmt::Debug
representation of the decodedmessage to stdout.
janus_cli
uses some of the scaffolding from thejanus
serverbinaries for option parsing, and so it used to expect to be provided a
configuration file with logging and particularly datbase config. That
isn't needed for the
decode-dap-message
command, so we also make theconfig file path optional.
Resolves #441