-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
refactor common proto resource (un)marshallers into generic functions #41562
Conversation
|
||
cp := proto.Clone(r).(T) | ||
//nolint:staticcheck // SA1019. Id is deprecated, but still needed. | ||
cp.GetMetadata().Id = 0 |
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.
AFAIK no one is using this anymore
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.
I think it's fine to leave this for now, we'll be removing Id entirely in the near future
lib/services/resource_test.go
Outdated
|
||
unmarshalled, err := tc.unmarshalFunc(marshaled) | ||
require.NoError(t, err) | ||
require.Empty(t, cmp.Diff(resource, unmarshalled, cmpopts.IgnoreUnexported(vnet.VnetConfig{}, vnet.VnetConfigSpec{}, headerv1.Metadata{}))) |
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.
require.Empty(t, cmp.Diff(resource, unmarshalled, cmpopts.IgnoreUnexported(vnet.VnetConfig{}, vnet.VnetConfigSpec{}, headerv1.Metadata{}))) | |
require.Empty(t, cmp.Diff(resource, unmarshalled, protocmp.Transform())) |
@nklaassen See the table below for backport results.
|
…ctions Backport #41562 to branch/v15
…ctions Backport #41562 to branch/v14
We have many implementations of resource marshallers and unmarshallers that are basically identical aside from the types. Most developers just find an existing resource and copy whatever it does.
This PR adds new generic marshal and unmarshal functions that should work for all new RFD153-style resources based on protobuf types generated with
buf
. The recommendation for new types going forward is to useservices.MarshalProtoResource
andservices.UnmarshalProtoResource
for your type.services.FastMarshalProtoResourceDeprecated
andservices.FastUnmarshalProtoResourceDeprecated
were also added since 7 of the new types already wrote their (un)marshal functions based onutils.FastMarshal
, which does not support some protobuf features likeoneof
. If someone takes the time to check if it would be safe to convert these types to the new protojson-based marshaler, they could potentially be converted.For now in this PR, I am keeping all functionality identical and just refactoring common code into shared functions. The one exception to this is MarshalAccessMonitoringRule which previously just called
utils.FastMarshal
without considering theMarshalOption
s, it now usesFastMarshalProtoResourceDeprecated
which does respect the marshal options.