Metadata (protos/packet) should be typed in Hardware

displague commented 3 years ago

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 commented 3 years ago

/cc @invidian (for Terraform provider relevance)

displague commented 3 years ago

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.

gianarb commented 3 years ago

@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?

jacobweinstock commented 1 year ago

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

tinkerbell / tink