tink icon indicating copy to clipboard operation
tink copied to clipboard

Metadata (protos/packet) should be typed in Hardware

Open displague opened this issue 4 years ago • 3 comments

Expected Behaviour

Metadata (from the metadata package) is typed within the Hardware type used by the Hardware client.

import (
        "github.com/tinkerbell/tink/protos/hardware"
        "github.com/tinkerbell/tink/protos/metadata"
)
...
	hwReq := &hardware.Hardware{
		Network: &hardware.Hardware_Network{},
		Id:       "uuid",
		Version:  0,
		Metadata: &metadata.Metadata{
		        Instance: &metadata.Metadata_Instance{},
	        }
	}
	pushReq := &hardware.PushRequest{data: hwReq}

Current Behaviour

Metadata (from the packet package) is a string within the Hardware type used by the Hardware client.

import (
        "github.com/tinkerbell/tink/protos/hardware"
        "github.com/tinkerbell/tink/protos/packet"
)
...
        packetMetadata := &packet.Metadata{
		Instance: &packet.Metadata_Instance{},
	}
	hwReq := &hardware.Hardware{
		Network: &hardware.Hardware_Network{},
		Id:       "uuid",
		Version:  0,
		Metadata: packetMetadata.String(),
	}
	pushReq := &hardware.PushRequest{data: hwReq}

As a Tinkerbell client implementor, creating a Hardware object, it is not expected that the client interface should require a string interpretation of the Metadata type from the protos/packet package.

Possible Solution

  • Rename protos/packet to protos/metadata.
  • Define the Hardware type's Metadata as a Metadata type (not a string) using protobuf imports (https://developers.google.com/protocol-buffers/docs/proto3) and optional/repeated elements (https://developers.google.com/protocol-buffers/docs/encoding#optional)

This may create a bc-break, so the PR should include this label.

Context

I'm investigating a Crossplane client for Tinkerbell (https://github.com/displague/crossplane-provider-tinkerbell/commit/cc989d6dbbe5e5ca16711f44b1a5c9b80a878ddf#diff-261eb121ddd5a8cf59bab09ef855945be48c5730d2f4687a1ee8575e8675eaa0R171-R192).

This would also benefit the Tinkerbell Terraform provider, which currently passes json and yaml blobs around (https://github.com/tinkerbell/terraform-provider-tinkerbell/blob/28bd5d0/tinkerbell/resource_hardware_test.go#L25-L93).

Your Environment

Go importing tink at 384fe30ce2a4

displague avatar Nov 07 '20 06:11 displague

/cc @invidian (for Terraform provider relevance)

displague avatar Nov 07 '20 06:11 displague

Is metadata intentionally a string to stay unbiased about the metadata format? Shouldn't Tinkerbell have a bias on the metadata format (a bias to follow hegel)?

This would be helpful for adding cloud-init support for the Tinkerbell metadata service.

displague avatar Nov 07 '20 06:11 displague

@mmlb is it part of the proposal you raised around instance? Can you help us to figure how if @displague has a reasonable request or if it is like it is for good reasons?

gianarb avatar Dec 10 '20 14:12 gianarb

packetMetadata.String() does not exist anymore. @displague if you feel this is still needed, please reopen.

jacobweinstock avatar Dec 23 '22 02:12 jacobweinstock