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

The fake client Status().Update() should not store object meta #2474

Closed
berlin-ab opened this issue Sep 7, 2023 · 12 comments · Fixed by #2479
Closed

The fake client Status().Update() should not store object meta #2474

berlin-ab opened this issue Sep 7, 2023 · 12 comments · Fixed by #2479
Labels
kind/bug Categorizes issue or PR as related to a bug. kind/support Categorizes issue or PR as a support question.

Comments

@berlin-ab
Copy link
Contributor

berlin-ab commented Sep 7, 2023

obj.SetAnnotations(map[string]string{
	"foo": "bar",
})
			
err = testClient.Status().Update(ctx, obj)
require.NoError(t, err)

_ = testClient.Get(ctx, k8sclient.ObjectKeyFromObject(obj), obj)
require.Equal(t, cluster.Annotations["foo"], "bar")

This test fails when using a real client, but succeeds when using the fake client.

@berlin-ab
Copy link
Contributor Author

The same is true of labels.

@troy0820
Copy link
Member

troy0820 commented Sep 7, 2023

/kind support
What version of controller runtime is this?
Also if you put that Get in a new “obj” var and not shadow it, does this happen?

@k8s-ci-robot k8s-ci-robot added the kind/support Categorizes issue or PR as a support question. label Sep 7, 2023
@troy0820
Copy link
Member

troy0820 commented Sep 7, 2023

 _ = testClient.Get(ctx, k8sclient.ObjectKeyFromObject(obj), obj)
require.Equal(t, cluster.Annotations["foo"], "bar")

This also doesn’t seem to test obj but asserts that the cluster.Annotations[“foo”] is equal to bar. Is this obj.Annotations[“foo”]?

@berlin-ab
Copy link
Contributor Author

This also doesn’t seem to test obj but asserts that the cluster.Annotations[“foo”] is equal to bar. Is this obj.Annotations[“foo”]?

Sorry, I was changing some variable names to make it less specific to my situation and missed one.

@berlin-ab
Copy link
Contributor Author

What version of controller runtime is this?

v0.15.2

@berlin-ab
Copy link
Contributor Author

I've modified the fake client test suite to show an example:

https://github.com/kubernetes-sigs/controller-runtime/pull/2475/files

@berlin-ab
Copy link
Contributor Author

Fake client [It] should not change non-status field of typed objects that have a status subresource on status update
/Users/aberlin/workspace/controller-runtime/pkg/client/fake/client_test.go:1445

  [FAILED] Expected
      <string>:   &v1.Node{
        	TypeMeta: {Kind: "Node", APIVersion: "v1"},
        	ObjectMeta: v1.ObjectMeta{
        		... // 8 identical fields
        		DeletionTimestamp:          nil,
        		DeletionGracePeriodSeconds: nil,
      - 		Labels:                     nil,
      + 		Labels:                     map[string]string{"some-label-key": "some-label-value"},
      - 		Annotations:                nil,
      + 		Annotations:                map[string]string{"some-annotation-key": "some-annotation-value"},
        		OwnerReferences:            nil,
        		Finalizers:                 nil,
        		ManagedFields:              nil,
        	},
        	Spec:   {PodCIDR: "old-cidr"},
        	Status: {NodeInfo: {MachineID: "machine-id-from-status-update"}},
        }
      
  to be empty
  In [It] at: /Users/aberlin/workspace/controller-runtime/pkg/client/fake/client_test.go:1480 @ 09/07/23 22:55:45.781

@troy0820
Copy link
Member

troy0820 commented Sep 8, 2023

/kind bug

Thanks for updating the issue.

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 8, 2023
@troy0820
Copy link
Member

troy0820 commented Sep 8, 2023

I'm trying to see if this is true for only core types and not custom resource types. Not to say that you shouldn't be able to use this on core types but it will help to understand the scope of this.

There is a test where unstructured.Unstructured works as you would expect. client-go has a fake with updateStatus that handles this.

@sbueringer @alvaroaleman any ideas?

@berlin-ab
Copy link
Contributor Author

I'm trying to see if this is true for only core types and not custom resource types.

I noticed this issue while working with a custom resource type.

@troy0820
Copy link
Member

troy0820 commented Sep 8, 2023

@berlin-ab The PR will hopefully address the issue with the test case you provided. The other issue you mentioned may be coupled with issue #2478 as well.

@berlin-ab
Copy link
Contributor Author

@troy0820 Thanks for the really quick response. I really appreciate that this fake client has folks helping to make it more real/correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. kind/support Categorizes issue or PR as a support question.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants