MEN-3996 ws/shell for Remote Terminal. #94
Conversation
ChangeLog:none Signed-off-by: Peter Grzybowski <peter@northern.tech>
merlin-northern
force-pushed
the
remote_terminal_ws_shell_module
branch
from
e24b136
to
cfa2eb8
Compare
There was a problem hiding this comment.
To be honest, this wasn't quite what I had in mind.
My intention with the ProtoMsg definition is that we have a common and minimalistic application layer (on top of websocket) for all future kinds of messages that we communicate over a websocket.
So, for all shell specific messages (ProtoMsg.Header.Proto == ProtoTypeShell should apply some protocol specific logic for decoding the message body and what fields and properties should be set in the header.
Thus, what I was expecting in the shell sub-package are declarations of different MsgType headers, and a struct definition for each MsgType (that requires a message body) for encoding/decoding the ProtoMsg.Body.
| // * CommandData for the stdout / stderr coming from the terminal and keystrokes from the UI | ||
| // * StartSession for the session start | ||
| // * StopSession for stopping of the session |
There was a problem hiding this comment.
This is shell specific. This package is supposed to be generic.
| // * MenderShell for remote terminal | ||
| // * MenderConfiguration for configuration of devices | ||
| // * MenderMonitoring for the stats and monitoring |
There was a problem hiding this comment.
This also seems superfluous as the documentation for the different types should be placed by their definition. (Notice that this field is of ProtoType).
| // Status is the indicator of whether the response contains error or other | ||
| // conditions (required). | ||
| // E.g.: for the remote terminal case a StartSession MsgType maye result in | ||
| // an error due to mas session limit reached. | ||
| // * Normal message carries a data from successful processing | ||
| // * Error the given message returned error | ||
| // * ProtocolError higher level protocol error | ||
| Status int `msgpack:"state"` |
There was a problem hiding this comment.
I'd argue this should be in the Properties. The reason being that a very small set of MsgType for a given Proto would actually set this field, and for some future Protos a status might not make sense at all.
| type Protocol interface { | ||
| DeEncapsulate(m *ProtoMsg) (interface{}, error) | ||
| Encapsulate(m interface{}) (*ProtoMsg, error) | ||
| } | ||
|
|
||
| var protoMap = map[ProtoType]Protocol{} |
There was a problem hiding this comment.
I don't understand the purpose of this interface. All messages holding shell data should be of ProtoTypeShell, thus this map holds at most one value.
| ErrorMessage | ||
| ) | ||
|
|
||
| var ( |
There was a problem hiding this comment.
| var ( | |
| const ( |
| type MenderShellMessage struct { | ||
| //type of message, used to determine the meaning of data | ||
| Type string `json:"type" msgpack:"type"` | ||
| //session id, as returned to the caller in a response to the MessageTypeSpawnShell | ||
| //message. | ||
| SessionId string `json:"session_id" msgpack:"session_id"` | ||
| //message status, currently normal and error message types are supported | ||
| Status MenderShellMessageStatus `json:"status_code" msgpack:"status_code"` | ||
| //the message payload, if | ||
| // * .Type===MessageTypeShellCommand interpreted as keystrokes and passed | ||
| // to the stdin of the terminal running the shell. | ||
| // * .Type===MessageTypeSpawnShell interpreted as user_id and passed | ||
| // to the session.NewMenderShellSession. | ||
| Data []byte `json:"data" msgpack:"data"` | ||
| } |
There was a problem hiding this comment.
This is overlapping with the ProtoMsg definition.
Type<->Header.MsgTypeSessionId<->Header.SessionIDData<->Body
| type MenderShellProtocol struct { | ||
| version string | ||
| } |
There was a problem hiding this comment.
Again, I don't understand the purpose of this interface. Why can't we use ProtoMsg?
We discussed in the design document that a Version could be given as a ProtoMsg.Header.Properties.
basically same here. let me think on how to restructure to make it more to your taste :) |
|
FWIW, I fully agree with what @alfrunes said. Let's try to keep this library high-level, generic, without any specific use-case implementation (shell, in this case). |
ChangeLog:none
Signed-off-by: Peter Grzybowski peter@northern.tech