Fix `could not find WorkloadDefinition` issue #208

zzxwill · 2020-09-16T16:26:09Z

When WorkloadDefinition doesn't follow the pattern
. or there are different
workloadDefinitions pointing to the same workload Schema,
it will hit the issue of could not finding workloaddefinition.
Fix issue #207, feature #195.

Signed-off-by: zzxwill [email protected]

zzxwill · 2020-09-16T16:40:16Z

I will add unit-test and e2e-test tomorrow to make the PR ready for review.

#207 (comment) is an introduction and comparison with the solution of #195.

wonderflow · 2020-09-17T03:27:54Z

pkg/oam/util/helper.go

@@ -226,7 +228,11 @@ func PassLabelAndAnnotation(parentObj oam.Object, childObj labelAnnotationObject

 // GetCRDName return the CRD name of any resources
 // the format of the CRD of a resource is <kind purals>.<group>
+// Now the CRD name of a resource could also be defined as `metadata.annotations.definition.oam.dev/name`


Suggested change

// Now the CRD name of a resource could also be defined as `metadata.annotations.definition.oam.dev/name`

// Now the CRD name of a resource could also be defined as `definition.oam.dev/name` in `metadata.annotations`

wonderflow · 2020-09-17T03:28:34Z

pkg/oam/util/helper.go

 func GetCRDName(u *unstructured.Unstructured) string {
+	if crdName, ok := u.GetAnnotations()[DefinitionAnnotation]; ok {


using index like this will easily cause panic if the map returned is nil

wonderflow · 2020-09-17T03:30:27Z

pkg/oam/util/helper.go

@@ -39,6 +39,8 @@ var (
 const (
 	// TraitPrefixKey is prefix of trait name
 	TraitPrefixKey = "trait"
+	// DefinitionAnnotation is the annotation of filed `metadata.annotations` for WorkloadDefinition and Workload


Suggested change

// DefinitionAnnotation is the annotation of filed `metadata.annotations` for WorkloadDefinition and Workload

// DefinitionAnnotation is one automatically generated annotation of workload and trait to indicate which workloadDefinition/ traitDefinition it is generated

wonderflow · 2020-09-18T03:07:33Z

pkg/oam/util/helper.go

@@ -39,6 +39,9 @@ var (
 const (
 	// TraitPrefixKey is prefix of trait name
 	TraitPrefixKey = "trait"
+	// DefinitionAnnotation is one automatically generated annotation of workload and trait to indicate which
+	//workloadDefinition/traitDefinition it is generated by
+	DefinitionAnnotation = "definition.oam.dev/name"


Maybe put this annotation const to types.go is better

wonderflow · 2020-09-18T08:02:46Z

pkg/oam/util/helper.go

@@ -226,7 +226,13 @@ func PassLabelAndAnnotation(parentObj oam.Object, childObj labelAnnotationObject

 // GetCRDName return the CRD name of any resources
 // the format of the CRD of a resource is <kind purals>.<group>
+// Now the CRD name of a resource could also be defined as `definition.oam.dev/name` in `metadata.annotations`
 func GetCRDName(u *unstructured.Unstructured) string {


Please also refactor the function Name, it should not be GetCRDName , name it GetDefinitionName seems better

wonderflow

LGTM, In this PR, we add an annotation into workload to show which Definition it is generated from.

When there is spec.type in Workload, an annotation will set for the workload which can help find the WorkloadDefinition. Fix issue crossplane#207, related to feature crossplane#195. Co-authored-by: Sun Jianbo <[email protected]> Signed-off-by: zzxwill <[email protected]>

Signed-off-by: zzxwill <[email protected]>

ryanzhang-oss · 2020-09-21T07:04:34Z

pkg/webhook/v1alpha2/component/mutating_handler.go

@@ -22,6 +22,8 @@ import (
 	"fmt"
 	"net/http"

+	"github.com/crossplane/oam-kubernetes-runtime/pkg/oam"


please move this with the local

ryanzhang-oss · 2020-09-21T07:07:05Z

pkg/webhook/v1alpha2/component/mutating_handler.go

-		workload.SetAnnotations(obj.GetAnnotations())
+		// Add another annotation DefinitionAnnotation which can mark the name of WorkloadDefinition
+		workload.SetAnnotations(util.MergeMap(obj.GetAnnotations(), map[string]string{oam.DefinitionAnnotation: workloadType}))
+		mutatelog.Info("Set annotation definition.oam.dev/name for workload", "annotation value", workloadType)


this seems to be a debugging log for the testing purpose only

Webhook is a little bit for local debug, so I write the log to make sure it takes effect. As you recommended to use the existing label below, I will remove the annotation setting and the log. Thanks.

ryanzhang-oss · 2020-09-21T07:07:43Z

pkg/webhook/v1alpha2/component/mutating_handler.go

@@ -125,7 +127,9 @@ func (h *MutatingHandler) Mutate(obj *v1alpha2.Component) error {
 		// copy namespace/label/annotation to the workload and add workloadType label
 		workload.SetNamespace(obj.GetNamespace())
 		workload.SetLabels(util.MergeMap(obj.GetLabels(), map[string]string{WorkloadTypeLabel: workloadType}))
-		workload.SetAnnotations(obj.GetAnnotations())
+		// Add another annotation DefinitionAnnotation which can mark the name of WorkloadDefinition
+		workload.SetAnnotations(util.MergeMap(obj.GetAnnotations(), map[string]string{oam.DefinitionAnnotation: workloadType}))


there is already a label to indicate the workloadType of the component. Where is this annotation with the same information used?

Oh,yes, using the existing WorkloadTypeLabel is better.

Will use the label, instead.

ryanzhang-oss · 2020-09-21T07:08:10Z

pkg/webhook/v1alpha2/component/component_test.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"fmt"

+	"github.com/crossplane/oam-kubernetes-runtime/pkg/oam"


move this to its group

ryanzhang-oss · 2020-09-21T07:08:50Z

pkg/oam/util/helper.go

+// Now the definition name of a resource could also be defined as `definition.oam.dev/name` in `metadata.annotations`
+func GetDefinitionName(u *unstructured.Unstructured) string {
+	if annotations := u.GetAnnotations(); annotations != nil {
+		if crdName, ok := annotations[oam.DefinitionAnnotation]; ok {


why not use the existing label and add this extra annotation?

Will use the label, thanks.

wonderflow · 2020-09-21T07:52:27Z

pkg/oam/util/helper.go

+// the format of the definition of a resource is <kind plurals>.<group>
+// Now the definition name of a resource could also be defined as `definition.oam.dev/name` in `metadata.annotations`
+func GetDefinitionName(u *unstructured.Unstructured) string {
+	if labels := u.GetAnnotations(); labels != nil {


We need to get labels now, not annotations

If tests still passed with this obviously bug, you need to write more cases I think.

If tests still passed with this obviously bug, you need to write more cases I think.

Sorry, the code was automatically reverted from Disk instead of Memory by Goland. My bad, fixed. Thanks.

pkg/webhook/v1alpha2/component/component_test.go

Signed-off-by: zzxwill [email protected] Signed-off-by: zzxwill <[email protected]>

zzxwill mentioned this pull request Sep 16, 2020

[BUG] Can't find workloadDefinition is name was not match pattern <puraral-kind>.<resource-group> #207

Closed

wonderflow suggested changes Sep 17, 2020

View reviewed changes

zzxwill closed this Sep 17, 2020

zzxwill force-pushed the definition branch from 7020fae to a760d93 Compare September 17, 2020 07:08

zzxwill changed the title ~~Fix could not find WorkloadDefinition~~ Fix could not find WorkloadDefinition issue Sep 18, 2020

zzxwill reopened this Sep 18, 2020

zzxwill marked this pull request as ready for review September 18, 2020 03:01

wonderflow reviewed Sep 18, 2020

View reviewed changes

zzxwill closed this Sep 18, 2020

zzxwill force-pushed the definition branch from 07d5a2b to a760d93 Compare September 18, 2020 03:10

zzxwill reopened this Sep 18, 2020

zzxwill force-pushed the definition branch 2 times, most recently from 5f60904 to b536e68 Compare September 18, 2020 06:38

wonderflow reviewed Sep 18, 2020

View reviewed changes

wonderflow approved these changes Sep 21, 2020

View reviewed changes

zzxwill and others added 3 commits September 21, 2020 14:32

Address comments and add unit-test

b92d0d1

Signed-off-by: zzxwill <[email protected]>

Fix goimports issue

a6c3e81

Signed-off-by: zzxwill <[email protected]>

zzxwill force-pushed the definition branch 2 times, most recently from e172e6d to a6c3e81 Compare September 21, 2020 06:36

Change function name and add unit-test

8786b7e

Signed-off-by: zzxwill <[email protected]>

ryanzhang-oss reviewed Sep 21, 2020

View reviewed changes

zzxwill force-pushed the definition branch from c1641b3 to 4d9e8e7 Compare September 21, 2020 07:49

wonderflow reviewed Sep 21, 2020

View reviewed changes

zzxwill force-pushed the definition branch from 4d9e8e7 to 8786b7e Compare September 21, 2020 07:53

wonderflow reviewed Sep 21, 2020

View reviewed changes

pkg/webhook/v1alpha2/component/component_test.go Outdated Show resolved Hide resolved

Removed maker annotation definition.oam.dev/name

94b0a8e

Signed-off-by: zzxwill [email protected] Signed-off-by: zzxwill <[email protected]>

zzxwill force-pushed the definition branch from 88ba4b8 to 94b0a8e Compare September 21, 2020 13:34

wonderflow merged commit 1537fb2 into crossplane:master Sep 21, 2020

zzxwill deleted the definition branch September 21, 2020 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `could not find WorkloadDefinition` issue #208

Fix `could not find WorkloadDefinition` issue #208

zzxwill commented Sep 16, 2020

zzxwill commented Sep 16, 2020

wonderflow Sep 17, 2020

wonderflow Sep 17, 2020

wonderflow Sep 17, 2020

wonderflow Sep 18, 2020

wonderflow Sep 18, 2020

wonderflow left a comment

ryanzhang-oss Sep 21, 2020

ryanzhang-oss Sep 21, 2020

zzxwill Sep 21, 2020

ryanzhang-oss Sep 21, 2020

wonderflow Sep 21, 2020

zzxwill Sep 21, 2020

ryanzhang-oss Sep 21, 2020

ryanzhang-oss Sep 21, 2020

zzxwill Sep 21, 2020

wonderflow Sep 21, 2020

wonderflow Sep 21, 2020

zzxwill Sep 21, 2020

	// Now the CRD name of a resource could also be defined as `metadata.annotations.definition.oam.dev/name`
	// Now the CRD name of a resource could also be defined as `definition.oam.dev/name` in `metadata.annotations`

		func GetCRDName(u *unstructured.Unstructured) string {
		if crdName, ok := u.GetAnnotations()[DefinitionAnnotation]; ok {

	// DefinitionAnnotation is the annotation of filed `metadata.annotations` for WorkloadDefinition and Workload
	// DefinitionAnnotation is one automatically generated annotation of workload and trait to indicate which workloadDefinition/ traitDefinition it is generated

Fix could not find WorkloadDefinition issue #208

Fix could not find WorkloadDefinition issue #208

Conversation

zzxwill commented Sep 16, 2020

zzxwill commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wonderflow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix `could not find WorkloadDefinition` issue #208

Fix `could not find WorkloadDefinition` issue #208