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

no DeepCopyObject() method for watch.Event type #18

Closed
fatih opened this issue Aug 4, 2017 · 9 comments
Closed

no DeepCopyObject() method for watch.Event type #18

fatih opened this issue Aug 4, 2017 · 9 comments

Comments

@fatih
Copy link

fatih commented Aug 4, 2017

Hi,

We're using the watch.Event type in our internal code base extensively. Some of the places encode it as well. After upgrading to latest client-go it stopped compiling because of the missing method DeepCopyObject. Seems like the following line is missing (https://github.com/kubernetes/apimachinery/blob/master/pkg/watch/watch.go#L54):

// k8s:deepcopy-gen:interfaces=k8s.io/kubernetes/runtime.Object

Can we generate this method for this type as well? I can open a PR but wasn't sure if that was intentionally like this.

@deads2k
Copy link
Collaborator

deads2k commented Aug 7, 2017

@sttts

@sttts
Copy link
Contributor

sttts commented Aug 7, 2017

@fatih please take a look at kubernetes/client-go#259. Sounds like the same problem, i.e. vendoring of the wrong apimachinery version.

@fatih
Copy link
Author

fatih commented Aug 7, 2017

@sttts Thanks for the info. But we're using now the latest apimachinery and I think this type should have this method anyway as it might be needed (we do now). Is there a way we could add these ?

@sttts
Copy link
Contributor

sttts commented Aug 7, 2017

@fatih got it. I am hestitant to turn watch.Event into an object. It is an internal type not meant to be serialized. It misses the GetObjectKind() schema.ObjectKind implementation as well. Why don't you use metav1.WatchEvent?

@fatih
Copy link
Author

fatih commented Aug 7, 2017

@sttts sure, but seems like it's not the same struct. It expects a runtime.RawExtension, not a runtime.Object.

It is an internal type not meant to be serialized

I'm using it for testing a Watch() call from the client. An example from the test I use:

ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	flusher, ok := w.(http.Flusher)
	if !ok {
		panic("need flusher!")
	}

	w.Header().Set("Transfer-Encoding", "chunked")
	w.WriteHeader(http.StatusOK)
	flusher.Flush()

	scheme := runtime.NewScheme()
	jsonSerializer := kubejson.NewSerializer(
		kubejson.DefaultMetaFactory, scheme, scheme, false)

	encoder := streaming.NewEncoder(w, jsonSerializer)
        // this is the place where it doesn't work anymore as `watch.Event()` doesn't have DeepCopyBoject() anymore
	if err := encoder.Encode(watch.Event{
		Type:   watch.Added,
		Object: obj,
	},
	); err != nil {
		panic(err)
	}

	flusher.Flush()
}))
defer ts.Close()

config := &rest.Config{
	Host: ts.URL,
}

client, err := NewClient(config)
if err != nil {
	t.Fatal(err)
}

// Certificates is a TPR we have implemented internally, but same logic applies to other resources
watcher, err := client.Certificates("default").Watch(metav1.ListOptions{})
if err != nil {
	t.Fatal(err)
}

ch := watcher.ResultChan()
event, ok := <-ch
if !ok {
	t.Errorf("should get a response, didn't have anything")
}

If there is any other solutions I'm happy to test it, but adding the DeepObjectObject() at least is useful for reasons like this (testing).

@sttts
Copy link
Contributor

sttts commented Aug 8, 2017

Compare vendor/k8s.io/apiserver/pkg/endpoints/handlers/watch.go:221. That's the http watch server in kube. It converts the watch.Event to a metav1.Event by essentially writing the embedded runtime.Object into the RawExtension's Object field (in Convert_versioned_InternalEvent_to_versioned_Event). Then the encoder will do the serialization normally.

@fatih
Copy link
Author

fatih commented Aug 10, 2017

Thanks @sttts.

@sttts
Copy link
Contributor

sttts commented Aug 10, 2017

@fatih does this work for you? Can we close the issue?

@fatih
Copy link
Author

fatih commented Aug 10, 2017

@sttts Sure, thanks for the help again.

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

No branches or pull requests

3 participants