fix: use metav1.Object instead of runtime.Object for more ergonomic annotation handling

[kevin1689-cloud]

related to #99

[CLAassistant]

All committers have signed the CLA.

[kevin1689-cloud]

Please take a look. Thanks!

[gary-lgy]

Besides avoiding the meta.Accessor overhead, another point of this refactor is to allow calling AddAnnotation without having to handle the returned error (given the current code paths, the old implementation of AddAnnotation would never return an error anyway).

The following is a possible implementation of AddAnnotation that satisfies the requirements:

func AddAnnotation(obj metav1.Object, key, value string) bool {
	if HasAnnotationKeyValue(obj, key, value) {
		return false
	}
	annotations := obj.GetAnnotations()
	if annotations == nil {
		annotations = make(map[string]string)
	}
	annotations[key] = value
	obj.SetAnnotations(annotations)
	return true
}

Naturally, since the return signature has changed, all the call sites need to be changed (which is a good thing because the old error handling code is merely unnecessary bloat).

[gary-lgy]

Pls see the above comment.

[kevin1689-cloud]

Pls see the above comment.

Got it, thanks for the example which is very clear. I will working on that.

[gary-lgy]

l

Pls see the above comment.

Got it, thanks for the example which is very clear. I will working on that.

Meanwhile, we're in the process of a major refactor (https://github.com/kubewharf/kubeadmiral/tree/refactor/unified-federated-type) and the code on the main branch will be changed significantly by next week. I suggest delaying this fix until the refactor is done, otherwise we will have to do a lot of rebasing. :)

[kevin1689-cloud]

Pls see the above comment.

Got it, thanks for the example which is very clear. I will working on that.

Meanwhile, we're in the process of a major refactor (https://github.com/kubewharf/kubeadmiral/tree/refactor/unified-federated-type) and the code on the main branch will be changed significantly by next week. I suggest delaying this fix until the refactor is done, otherwise we will have to do a lot of rebasing. :)

Got it, how about in this pull request we only avoiding the meta.Accessor overhead which only need change two files:

• pkg/controllers/util/annotation/annotation.go
• pkg/controllers/util/annotation/annotation_test.go

For the refactor which allow calling AddAnnotation without having to handle the returned error, we delaying it until the major refactor is done. After the major refactor is done, I can create another pull request to remove the returned error of AddAnnotation and change all the call sites.

I think by this way we can make some improvement(avoiding the meta.Accessor overhead) and minimize the rebasing work. What do you think about that? Thanks!

[gary-lgy]

Pls see the above comment.

Got it, thanks for the example which is very clear. I will working on that.

Meanwhile, we're in the process of a major refactor (refactor/unified-federated-type) and the code on the main branch will be changed significantly by next week. I suggest delaying this fix until the refactor is done, otherwise we will have to do a lot of rebasing. :)

Got it, how about in this pull request we only avoiding the meta.Accessor overhead which only need change two files:

• pkg/controllers/util/annotation/annotation.go
• pkg/controllers/util/annotation/annotation_test.go

For the refactor which allow calling AddAnnotation without having to handle the returned error, we delaying it until the major refactor is done. After the major refactor is done, I can create another pull request to remove the returned error of AddAnnotation and change all the call sites.

I think by this way we can make some improvement(avoiding the meta.Accessor overhead) and minimize the rebasing work. What do you think about that? Thanks!

Sounds good. Please fix the compile error and then LGTM.

[kevin1689-cloud]

Pls see the above comment.

Got it, thanks for the example which is very clear. I will working on that.

Meanwhile, we're in the process of a major refactor (refactor/unified-federated-type) and the code on the main branch will be changed significantly by next week. I suggest delaying this fix until the refactor is done, otherwise we will have to do a lot of rebasing. :)

Got it, how about in this pull request we only avoiding the meta.Accessor overhead which only need change two files:

• pkg/controllers/util/annotation/annotation.go
• pkg/controllers/util/annotation/annotation_test.go

For the refactor which allow calling AddAnnotation without having to handle the returned error, we delaying it until the major refactor is done. After the major refactor is done, I can create another pull request to remove the returned error of AddAnnotation and change all the call sites.
I think by this way we can make some improvement(avoiding the meta.Accessor overhead) and minimize the rebasing work. What do you think about that? Thanks!

Sounds good. Please fix the compile error and then LGTM.

Done. The compile error has been fixed by replace runtime.Object to metav1.Object in the file pkg/controllers/util/sourcefeedback/util.go. Please take a look.

[gary-lgy]

@kevin1689-cloud thanks!