What do you think about not directly exposing/using the internal gRPC structs for the command output? Instead, create a new clean "view" struct for the Machine where we can define the fields we want to show, the case, and the values format, e.g. for enums. We should probably use that new struct in the client as well as the public API so a user doesn't need to work with more low-level auto-generated structs from the proto.

It's a bit more work to convert between them but fully owning the api structs provides friendlier Go-native experience and allows to easily add custom methods to them if needed.
IDK, am I not using proto the right way?

I think I have an even better idea, even though this is hacky, but I think it will work.

We do own the protobuf definition and it is just Go code, so why don't we rewrite the generated protobuf files and just update the - in our eyes - correct json tags..., I don't think they are used for something else

that means reworking this PR in a go generate that becomes part of the build process.

and look it already exists: https://github.com/fatih/gomodifytags
(shold prolly need a little more work, but ....)

gomodifytags -file machine.pb.go -all -remove-tags json | gomodifytags -file /dev/stdin -all -add-tags json -add-options json=omitempty -transform pascalcase | less

til this is actually called pascalcase.

type MachineInfo struct {
        state         protoimpl.MessageState  `json:"State,omitempty"`
        sizeCache     protoimpl.SizeCache     `json:"SizeCache,omitempty"`
        unknownFields protoimpl.UnknownFields `json:"UnknownFields,omitempty"`

        Id       string         `protobuf:"bytes,1,opt,name=id,proto3" json:"Id,omitempty"`
        Name     string         `protobuf:"bytes,2,opt,name=name,proto3" json:"Name,omitempty"`
        Network  *NetworkConfig `protobuf:"bytes,3,opt,name=network,proto3" json:"Network,omitempty"`
        PublicIp *IP            `protobuf:"bytes,4,opt,name=public_ip,json=publicIp,proto3" json:"PublicIp,omitempty"`
}

is exactly what we want, and saves (a quite the hassle) redefining everything

I don't think they are used for something else

The serialised JSONs are stored in Corrosion in the machines table. So this will be a breaking change we need to account.

It fixes JSON keys but does nothing for Go ergonomics. The caller still receives *pb.IP, *pb.IPPort, *pb.MachineMember, custom Duration and Timestamp and have to call ToAddr(), ToPrefix(), handle nils and errors everywhere. In some places I even started ignoring the err returned by these conversion methods when I was sure the data was validated before. It's just so much boilerplate.

Proto types can't carry domain methods (Validate, Clone, Equal) and you still can't automatically attach methods like Equals on regenerated types without risk anyway.

I think we still want to maintain Go-native domain data types at least for commonly used resources such as Machine.

They also not always map 1:1 to the RPC wire data types. For example, we have Service that doesn't exist at the RPC level and is comprised of individual container calls fully on the client. This might change in the future and we can change the RPC wire level potentially without even affecting the user-facing Service.

Our client methods are also not always 1:1 mapping to the RPC methods because we often need to do the low level proxy/broadcast routine. So far we've been pretty successful at hiding these complexities from the user of the client but still provide a clear interface, for example, to specify if a particular action needs to run on a specific machine using a simple argument or a field in an options struct.

Now, it would be great to draw a clear line what we should mirror/duplicate and what not.

Mirroring all the proto types feels like it defeats the whole purpose of using proto in the first place. I don't know to be honest and I'm fine to not overthink it.

I would probably not bother to recreate type like UpdateMachineRequest, JoinClusterRequest, AddMachineRequest which are used only in one method and which are used very rarely. The only problem is inconsistency. But again we have much bigger fish to fry at the moment than to try to perfect these types.
I'm sure we will work out some intuition and maybe better ideas after working more with those.

The serialised JSONs are stored in Corrosion in the machines table. So this will be a breaking change we need to account.

ah, then this route is a no-go

The serialised JSONs are stored in Corrosion in the machines table. So this will be a breaking change we need to account.

Very true. Hence the WIP status here, and good to have some docs on subject. Also saying somewhere that json outout just like the text output is not stable seems like a good starting point.

just like the text output is not stable seems like a good starting point.

What do you mean when you say JSON is not stable? What scenario do you have in your mind you want to warn about?

user parsers json to do a thing, we update the json (public_key -> PublicKey), json parsing fails.

"Until version X, the JSON output can change in backwards incompatible ways between Uncloud versions" - or something like that.