Skip to content
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

Add tests to ensure no copy on assignment, fix map #95

Merged
merged 4 commits into from
Aug 31, 2021

Conversation

seena-stripe
Copy link
Collaborator

@seena-stripe seena-stripe commented Aug 25, 2021

Summary

This PR adds tests to ensure protoMap, protoList and protoMessage are not copying values on assignment and corrects protoMap. Yesterday I learned Skycfg previously did this sometimes, e.g. in the old implementation this would happen:

# v0.1.0 Behavior
msg1.r_string = ["a","b"]
a = msg1.r_string
msg2.r_string = msg1.r_string
a.append("c")

# a = [a, b, c]
# msg1.r_string = [a, b, c]
# msg2.r_string = [a, b]

(

case *starlark.List:
)

The re-implementation fixed this for lists but not for maps incidentally so I added tests to make this behavior explicit. AFAICT from testing starlark, starlark does not copy on assignment for lists and dictionaries (like python) and seems to be what is described in https://github.com/bazelbuild/starlark/blob/master/spec.md#value-concepts

Tests

Added tests when assigning skycfg and starlark values into a skycfg object

Copy link
Collaborator

@kathleen-stripe kathleen-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - one non-blocking comment. Thanks for fixing this behavior and adding test coverage!

go/protomodule/protomodule_map.go Outdated Show resolved Hide resolved
@com6056
Copy link

com6056 commented Mar 20, 2024

@seena-stripe you mentioned that the old implementation

case *starlark.List:
still has this issue, unfortunately we can't upgrade to the new implementation quite yet due to k8s still using gogoproto.

Do you happen to know how to fix this in the old implementation? I've tried a few things and couldn't quite get it working, was curious if you already knew how to solve it though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants