sdkserver: add functionality to set multiple game server annotations at once

[antiphp]

Is your feature request related to a problem? Please describe. When someone watches for game server changes, it is preferred to see one change, rather than many where it is hard to determine when the sequence of changes is finished.

Describe the solution you'd like I suggest to implement a function for the sdk server to set multiple annotations at once. Currently only SetAnnotation (singular) exists.

Describe alternatives you've considered Set an annotation that indicates that this is the last annotation.

Additional context Related code: https://github.com/googleforgames/agones/blob/main/pkg/sdkserver/sdkserver.go#L580

[frenchtoasters]

Hi @antiphp I would like to help out with this issue, I am new to the project and just had a few questions before jumping in.

Reading your solution it sounds like changing the gsAnnotations field to []map[string]string here https://github.com/googleforgames/agones/blob/2f1c2a8be8c9f7d3c643380e9a060368739b8fea/pkg/sdkserver/sdkserver.go#L124 then updating SetAnnotation (singular) to either append or overwrite.

Then adding a SetAnnotations (plural) that looks like func (s *SDKServer) SetAnnotations(_ context.Context, kv []*sdk.KeyValue) (*sdk.Empty, error).

Is that the line of thinking you would like here? Or do you think changing that property of the SDKServer to a slice would break something else? Aside from adding a test is there anywhere you would like this implemented with this?

[antiphp]

Hi,

the function signature for SetAnnotations (p) seems fine. One could think of making kv variadic, but it does not seem to be used that often in this project except for GRPC options.

What's the thinking behind append vs overwrite? In my opinion, the latter key will always overwrite, so []*sdk.KeyValue{{"foo":"bar"},{"foo":"baz"}} should always result in foo=baz. We could error but it doesn't feel right to add sophisticated logic to what is just a map and everyone expects it to act like a map. But note that this is only my opinion, I am not related to this project, so in the end someone else will review and decide.

Btw this ticket is related: https://github.com/googleforgames/agones/issues/3834 - Removal of annotations.

Thanks for taking the initiative!

[frenchtoasters]

Hi,

the function signature for SetAnnotations (p) seems fine. One could think of making kv variadic, but it does not seem to be used that often in this project except for GRPC options.

What's the thinking behind append vs overwrite? In my opinion, the latter key will always overwrite, so []*sdk.KeyValue{{"foo":"bar"},{"foo":"baz"}} should always result in foo=baz. We could error but it doesn't feel right to add sophisticated logic to what is just a map and everyone expects it to act like a map. But note that this is only my opinion, I am not related to this project, so in the end someone else will review and decide.

Btw this ticket is related: #3834 - Removal of annotations.

Thanks for taking the initiative!

The idea to use append vs overwrite was so that we could handle updating multiple different annotations in the same function i.e. we would get []*sdk.KeyValue{{"foo":"bar"},{"some":"thing"}} and we needed both those annotations applied to the object. However this might just be a misunderstanding on my part of how the project invokes these under the hood.

I was thinking originally that we just wanted to be able to add multiple annotations that are passed to the function at once to a GameServer object. However it seems like the ask is more of not only add the annotations but to update the values of them as well. Which if that is the case I would agree completely that overwrite is the way to go instead of append.

[antiphp]

:wave: Are you still interested in opening a PR for that @frenchtoasters ? Otherwise I am thinking about doing it by myself in the near future.

[antiphp]

Thoughts around this PR:

1. Passing an empty string value could be used to remove the annotation (related to #3834), but it is a valid value, and SetAnnotation (singular) does not work like it, so it would be inconsistent.
2. I would have loved to have SetAnnotations() accept variadic args for KeyValue (would make the introduction of KeyValues unnecessary), but I think it's not possible with Protobuf
3. Is the same required for labels? (SetLabels) (not from my end yet)

[github-actions[bot]]

'This issue is marked as Stale due to inactivity for more than 30 days. To avoid being marked as 'stale' please add 'awaiting-maintainer' label or add a comment. Thank you for your contributions '

[github-actions[bot]]

This issue is marked as obsolete due to inactivity for last 60 days. To avoid issue getting closed in next 30 days, please add a comment or add 'awaiting-maintainer' label. Thank you for your contributions