microsoft / opengcs

Moved to https://github.com/microsoft/hcsshim/tree/master/internal/guest. If you wish to open PRs/submit issues please do so against https://github.com/microsoft/hcsshim.
MIT License
83 stars 41 forks source link

Simplify the bridge request/return pattern #317

Closed jterry75 closed 5 years ago

jterry75 commented 5 years ago

This change makes it easier for the rquest/response pattern to return a result or error. This is perferred over the w.Write and w.Error pattern where a response must be written and then an early return. This makes the code easier to read and control flow easier to understand.

jstarks commented 5 years ago

This looks good, but instead of returning interface{}, I would suggest returning a type of

interface {
    GetBase() *MessageResponseBase
}

And then implement GetBase() on *MessageResponseBase to return itself.

This approach ensures that the functions return a response type (could still be the wrong one, of course), and it allows the caller of all these functions to copy the ActivityID field from the request rather than rely on each of the individual code paths to remember to do it.

jterry75 commented 5 years ago

This looks good, but instead of returning interface{}, I would suggest returning a type of ``` interface { GetBase() *MessageResponseBase }


And then implement GetBase() on *MessageResponseBase to return itself.

This approach ensures that the functions return a response type (could still be the wrong one, of course), and it allows the caller of all these functions to copy the ActivityID field from the request rather than rely on each of the individual code paths to remember to do it.

I'm sure I am missing something but how does this help? Not all functions return a *MessageResponseBase so having an interface with a GetBase() method doesn't help me to eventually serialize the entire structure. It would only allow me to serialize the *MessageResponseBase part of the structure.

jstarks commented 5 years ago

Take a look at https://github.com/microsoft/hcsshim/blob/master/internal/gcs/protocol.go.

jterry75 commented 5 years ago

Take a look at https://github.com/microsoft/hcsshim/blob/master/internal/gcs/protocol.go.

Got it thanks! PR Updated

jterry75 commented 5 years ago

34ths times the charm! @jstarks - PTAL