-
Notifications
You must be signed in to change notification settings - Fork 52
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
[Feature Branch] Support protobuf/msgpack serialization for cty Values #229
Conversation
fdcd784
to
b9a8108
Compare
b9a8108
to
914dafe
Compare
cc8fe50
to
ca6262e
Compare
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.
Overall this looks good. I left one small suggestion but I will add a test for it. As discussed internally I am leaving a comment to mark the approach as approved but this is a feature branch that we will keep open and iterate on.
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.
Left a question (although I know you probably have a good reason) but solid PR
LGTM
This commit introduces a protobuf-serialisable type for hcldec.Spec types. We need this be able to use another format to serialise them without using gob, as gob isn't supported anymore for cty.Type. The HCL2Spec function, implemented by all the plugins, are the ones responsible for transmitting the schema of their configurations to Packer, which as of v0.5.1 of the SDK, is using gob for serialisation, which has been removed from the cty library.
When a plugin describes its capabilities, it needs to advertise whether or not protobuf can be used in order for Packer to know which serialisation protocol to use for communicating with the plugin. To do so, we add a protocol_version attribute to the returned structure, which is now set to v2, in order for Packer to know that the plugin can use that.
As follow-up to the introduction of the protobufs for HCLSpec, we introduce a new environment variable and code to use those structures, so we don't use gob for serialising HCLSpecs. This should make the plugins and packer able to transmit data over-the-wire without using gob for the most part (the communicators still use it, and will probably need some work to replace).
``` ~> go test ./plugin/... -v === RUN TestPluginServerRandom --- PASS: TestPluginServerRandom (0.00s) === RUN TestSet --- PASS: TestSet (0.00s) === RUN TestSetProtobufArgParsing === RUN TestSetProtobufArgParsing/no_--protobuf_argument_provided === RUN TestSetProtobufArgParsing/providing_--protobuf_as_first_argument === RUN TestSetProtobufArgParsing/providing_--protobuf_as_last_argument === RUN TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument --- PASS: TestSetProtobufArgParsing (0.00s) --- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s) PASS ok github.com/hashicorp/packer-plugin-sdk/plugin 0.249s ```
The upper index bound for a slice is cap(args) we can safely retun appended slices ``` ~> go test -count=1 ./plugin/... -v === RUN TestPluginServerRandom --- PASS: TestPluginServerRandom (0.00s) === RUN TestSet --- PASS: TestSet (0.00s) === RUN TestSetProtobufArgParsing === RUN TestSetProtobufArgParsing/no_--protobuf_argument_provided === RUN TestSetProtobufArgParsing/providing_--protobuf_as_first_argument === RUN TestSetProtobufArgParsing/providing_--protobuf_as_last_argument === RUN TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument --- PASS: TestSetProtobufArgParsing (0.00s) --- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s) PASS ok github.com/hashicorp/packer-plugin-sdk/plugin 0.244s ```
``` ~> go test ./plugin/... -v === RUN TestPluginServerRandom --- PASS: TestPluginServerRandom (0.00s) === RUN TestSet --- PASS: TestSet (0.00s) === RUN TestSetProtobufArgParsing === RUN TestSetProtobufArgParsing/no_--protobuf_argument_provided === RUN TestSetProtobufArgParsing/providing_--protobuf_as_first_argument === RUN TestSetProtobufArgParsing/providing_--protobuf_as_last_argument === RUN TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument === RUN TestSetProtobufArgParsing/providing_--protobuf_multiple_times --- PASS: TestSetProtobufArgParsing (0.00s) --- PASS: TestSetProtobufArgParsing/no_--protobuf_argument_provided (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_first_argument (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_last_argument (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_as_middle_argument (0.00s) --- PASS: TestSetProtobufArgParsing/providing_--protobuf_multiple_times (0.00s) PASS ok github.com/hashicorp/packer-plugin-sdk/plugin 0.250s ```
28e1cb0
to
e0c1a4f
Compare
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
This PR aims to be a PoC/base for later developments on the wire-protocol for all the components that plugin expose.
We introduce a protobuf-serialisable data structure for
hcldec.HCLSpec
, which is used by all components of a plugin for describing the schema of what the plugin uses, and offers Packer a type to use to decode cty values from HCL code.In doing so, we also explored using alternative serialisation formats for datasources, which were the only components leveraging cty's gob serialisation capabilities, and replace them with JSON/msgpack.
These changes make the SDK capable to communicate over-the-wire on recent hcl/cty versions, without needing to use the fork/replace for gob support.