change Clone attribute of PostgresSpec to *CloneDescription #1020
Conversation
hlihhovac
requested review from
avaczi,
CyberDem0n,
erthalion,
FxKu,
Jan-M,
RafiaSabih and
sdudoladov
as code owners
hlihhovac
changed the title
Co-authored-by: Felix Kunde <felix-kunde@gmx.de>
|
👍 |
| ConnectionPoolerDefaultMemoryRequest: "100Mi", | ||
| ConnectionPoolerDefaultMemoryLimit: "100Mi", | ||
| NumberOfInstances: int32ToPointer(1), | ||
| newCluster := func() *Cluster { |
There was a problem hiding this comment.
@hlihhovac @pashagolub Why these changes are necessary for the CloneDescription?
There was a problem hiding this comment.
Ping, should this be removed for the scope of having a smaller PR=
There was a problem hiding this comment.
@Jan-M do you want to remove test completely?
There was a problem hiding this comment.
@erthalion these changes fix error during manual creation of postgresqls.acid.zalan.do CRD.
There was a problem hiding this comment.
@erthalion these changes fix error during manual creation of postgresqls.acid.zalan.do CRD.
That's the goal of the whole PR, isn't it? I'm just trying to figure out if there is any mysterious value in changing unit test that seems to be completely unrelated (connection pooler test).
There was a problem hiding this comment.
That unit test was broken with new functionality:
--- FAIL: TestConnectionPoolerSynchronization (0.00s)
Objects for testing before changes were static objects, after change introduced they became pointers... So before changes, the test just copies static objects and takes the address of it. And now copying pointer will still point to the same object! So tests always ruined the same underlying object with mock copies.
That's why all new mocked objects now created using New(), but not copying.
I hope it's clear enough :)
There was a problem hiding this comment.
But only CloneDescription became a point, right? And it's not involved in this test, so still sounds strange for me. But ok, at least it's not included by mistake.
There was a problem hiding this comment.
The thing is the test itself was written for static properties. As soon as we've added pointers the simple copying became a problem. That's why we do not copy mock objects but create them separately so they do not interfere
|
👍 |
|
Thanks @hlihhovac and @pashagolub for your contribution! |
|
Thanks for cooperation! |
|
Thx everybody! |
When creating
postgresqls.acid.zalan.doCRD using client-go andtype PostgresSpec, process fails infunc (c *postgresqls) Create(ctx context.Context, postgresql *v1.Postgresql, opts metav1.CreateOptions) (result *v1.Postgresql, err error)due to the missingPostgresSpec.Cloneattribute.This was resolved changing
Cloneattribute to pointer*CloneDescriptioninPostgresSpecstruct.Other changes were required to adopt for above change.