Cometd.pm: enable basic CLI functionality #1146
Conversation
| if ($httpResponse) { | ||
| # Add any additional pending events | ||
| push @{$out}, ( $manager->get_pending_events( $httpClient->clid ) ); | ||
|
|
||
| # Add special first event for /meta/(re)connect if set | ||
| # Note: calling first_event will remove the event from httpClient | ||
| if ( my $first = $httpClient->first_event ) { | ||
| unshift @{$out}, $first; | ||
| } | ||
| # Add special first event for /meta/(re)connect if set | ||
| # Note: calling first_event will remove the event from httpClient | ||
| if ( my $first = $httpClient->first_event ) { | ||
| unshift @{$out}, $first; | ||
| } |
There was a problem hiding this comment.
Please describe what you're doing here, why you're doing it, and why it's working without breaking things.
There was a problem hiding this comment.
As far as I've been able to determine, $httpResponse will always eval to true here for HTTP clients and always to false for CLI clients. The original code that was outside the conditional would fail for CLI clients and result in a response never being returned to the CLI plugin (and thus the client).
This change moves the existing HTTP client specific code under the conditional for $httpResponse so it's not executed for CLI clients.
In currently shipping code, the CLI plugin will send any commands beginning with The basic logic for this PR is to first determine whether the client being handled in Once
Comet/JSON is formatted and returned by existing code outside of this PR. The issue is that this code is never reached. This PR (or something like it) re-enables it. Lines 749 to 752 in 4bd557e |
| @@ -658,7 +660,7 @@ sub sendResponse { | |||
| else { | |||
| # For CLI, don't send anything if there are no events | |||
| if ( scalar @{$out} ) { | |||
| sendCLIResponse( $httpClient, $httpResponse, $out ); | |||
| sendCLIResponse( $httpClient, $out ); | |||
There was a problem hiding this comment.
What about this change? I see that sendCLIRequest() would only accept two parameters. So this change makes sense - except that you're now passing different data to it, not less. How would that now work if it wasn't broken before?
There was a problem hiding this comment.
Ok, I believe you gave the answer already: that code was never reached. So essentially this should have been supported from the early days. But nobody used it or understood it, therefore nobody complained about it being broken?...
Could you please add a few lines to the CLI documentation, too? Explaining how to use the CLI in JSON mode:
https://github.com/LMS-Community/lms-community.github.io/blob/main/docs/reference/cli/introduction.md
https://github.com/LMS-Community/lms-community.github.io/blob/main/docs/reference/cli/using-the-cli.md
There was a problem hiding this comment.
Thanks for the review and patience on this Michael!
I meant to note that the $httpClient variable is unfortunately named here for CLI requests but I left it as-is to minimize the changes.
I understand your concern on breaking existing HTTP functionality. Looking at my sodface.com web server logs, I have 6 or 7 people using my 8.5.3 LMS package on Alpine. I could push this change to my package first and post in the Alpine forum thread about it, that way there's at least a little more testing done? Or if you consider 9.x testing already then maybe that's not necessary?
There was a problem hiding this comment.
Yes, after better understanding the change I'd be willing to merge to LMS9. As it's in such a central piece of LMS, any issue should show up quickly, I think.
There was a problem hiding this comment.
Wanted to clarify this PR only addresses:
/meta/handshake to get a clientID assigned, and
/slim/request to do one off commands/queries with json responses
Some of the other Cometd functions are probably still broken for CLI clients, like subscribe and unsubscribe. I was trying to keep this simple and restore basic /slim/request function. Is that ok for now?
There was a problem hiding this comment.
IMHO this PR is about getting JSON structured data over CLI, not about implementing all cometd features in CLI. cometd is a transfer protocol, like the telnet style CLI's connection is. So we're good. You might be the only one to use the CLI this way anyway. Because: why use it when you can use JSON/RPC? 😝
|
Oh, one more ask: please add a line to the changelog, referring this PR (and thanking yourself 😂). Thanks! |
Updated changelog but not in a separate commit, hope that's ok. |
Leverage existing code to enable basic CLI functionality. This allows /slim/request over CLI port 9090 to return JSON formatted responses.
See also: https://forums.slimdevices.com/forum/developer-forums/developers/1721698-comet-json-over-cli-9090