weaveworks / weave-gitops-enterprise

This repo provides the enterprise level features for the weave-gitops product, including CAPI cluster creation and team workspaces.
https://docs.gitops.weave.works/
Apache License 2.0
160 stars 30 forks source link

create-request annotations on GitopsClusters use snake_case #1485

Open foot opened 2 years ago

foot commented 2 years ago
TargetNamespace string     `protobuf:"bytes,3,opt,name=target_namespace,json=targetNamespace,proto3" json:"target_namespace,omitempty"`

Where (I think) data can be unmarshal'd using either camel or snake case (at least via the grpc-gateway), but marshaling uses snake_case..

Core has switched to using camelCase in field names, some recent discussion: https://github.com/weaveworks/weave-gitops/pull/2638#pullrequestreview-1085503496

The linked thread says we should always being using jsonpb for serializing protobuf stuff. Could we use that instead to generate the create-request annotation?

cc @bigkevmcd

foot commented 2 years ago

ofc we should be defensive about interpreting create-request anyway as it could be quite old.

foot commented 2 years ago

https://pkg.go.dev/github.com/golang/protobuf/jsonpb says its deprecated and we should use https://pkg.go.dev/google.golang.org/protobuf/encoding/protojson instead

bigkevmcd commented 2 years ago

The Google Guidelines on Protobufs https://developers.google.com/protocol-buffers/docs/style#message_and_field_names

Use CamelCase (with an initial capital) for message names – for example, SongServerRequest. Use underscore_separated_names for field names (including oneof field and extension names) – for example, song_name.

Do we know why we went against the Google guidelines?

foot commented 2 years ago

Do we know why we went against the Google guidelines?

We follow the guidelines in WGE, but stopped following them in core so we could re-use the go types generated by the protobuf files in the resty cli client. https://github.com/weaveworks/weave-gitops/pull/2572 . Following the guidelines means you should always marshal with jsonpb which reads the protobuf tag instead of the json tag. If you break guidelines and use camelCase in your fieldsnames then the json tag is also camelCased and you can use other marshalers like resty.

BUT! The main issue here is the http api that is generated for grpc-gateway is all camelCased, but we save the create-request annotation using the std. json lib which results in snake_cased JSON. So we have to do a bit of transforming when reading it.

Its all a bit confusing, maybe we can come up with some guidelines here.

bigkevmcd commented 2 years ago

We could change the marshaling if we need to?

bigkevmcd commented 2 years ago

https://gist.github.com/bigkevmcd/3e467fada2c76ebf4025cba61509c5d6 shows how we can change the marshaling to get snake_case