Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

add labels for workload to record component info #189

Merged
merged 3 commits into from
Aug 31, 2020

Conversation

captainroy-hy
Copy link
Contributor

  • add labels for workloads to record component info
  • add e2e test & unit test

Signed-off-by: roy wang [email protected]

add e2e test & unit test

Signed-off-by: roy wang <[email protected]>
Copy link
Member

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM , ping @ryanzhang-oss @zzxwill

oam.LabelAppComponent: acc.ComponentName,
oam.LabelAppComponentRevision: componentRevisionName,
}
w.SetLabels(labels)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make sure we won't override the existing labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I will use merge instead of override here.

add unit test

Signed-off-by: roy wang <[email protected]>
@@ -469,6 +477,17 @@ func fillValue(obj *unstructured.Unstructured, fs []string, val interface{}) err
return nil
}

func addWorkloadLabels(w *unstructured.Unstructured, labels map[string]string) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move this to util and change the name from "addWorkloadLabels" to "addLabels" as this is very generic. Also, I am not sure if this already exists somewhere in the library

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will check and move it.

@ryanzhang-oss
Copy link
Collaborator

Good job

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants