kr8s-org / kr8s

A batteries-included Python client library for Kubernetes that feels familiar for folks who already know how to use kubectl
https://kr8s.org
BSD 3-Clause "New" or "Revised" License
839 stars 45 forks source link

Fix setting spec of a resource from other resource #407

Closed leelavg closed 5 months ago

leelavg commented 5 months ago

ref: https://github.com/kr8s-org/kr8s/pull/401#discussion_r1634860377

leelavg commented 5 months ago

Works at 786a390 and fails at e2b04bc (or after #401). Will take sometime to get comfortable w/ tools used in the project, will revisit to find the issue.

jacobtomlinson commented 5 months ago

Thanks for this, it's really helpful to have a failing test like this. I've made some changes to the getter/setter and also made sure we only use the property throughout. I just pushed 5928daa onto this PR so I can get it merged in.

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.32%. Comparing base (87063fc) to head (5928daa). Report is 112 commits behind head on main.

Files with missing lines Patch % Lines
kr8s/_objects.py 90.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #407 +/- ## ========================================== + Coverage 94.61% 95.32% +0.70% ========================================== Files 29 29 Lines 3141 3592 +451 ========================================== + Hits 2972 3424 +452 + Misses 169 168 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

leelavg commented 5 months ago

I've made some changes to the getter/setter and also made sure we only use the property throughout.

  • ya, seems we can now reason about the flow. I still need to figure how your test worked when raw was returing a new dict but then the update to this dict still sticked to the object.
jacobtomlinson commented 5 months ago

My test passed because I was modifying objects that are nested within and so the pointer ends up pointing to the same dict even though it has been unwrapped and updated into a new dict. Your test failed because you're modifying the top level key.

Here's a toy example

class Foo:

    def __init__(self):
        self._data = {"inner": {"hello": "world"}}

    @property
    def data(self):
        d = {}  # Create a new dict
        d.update(**self._data)  # Copy all the keys and their value pointers into the new dict
        return d

    @data.setter
    def data(self, value):
        self._data = value
f = Foo()

id(f.data) == id(f._data)  # False
id(f.data["inner"]) == id(f._data["inner"])  # True

# Has no effect because it is modifying the copy
f.data["foo"] = "bar"
"foo" in f.data  # False

# Works because `"inner"` is pointed to by both the copy and the original
f.data["inner"]["fizz"] = "buzz"
"fizz" in f.data["inner"]  # True

In the fix we just return self._data directly so that id(f.data) == id(f._data) becomes True.

leelavg commented 5 months ago

Ah! right, thanks, missed it. .update isn't a deep copy.