You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/03/11 07:53:29 UTC
[GitHub] [incubator-yunikorn-k8shim] yangwwei opened a new pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
yangwwei opened a new pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239
sometimes we need to pass in the labels/annotations to the placeholder pods in order to make them more "like" the real pods. This helps certain cases, for example, if some namespace has sidecar container auto-injection enabled, we can leverage the annotation field to enable or disable the injection, just to comply with the real pod definition.
This PR adds labels/annotations to the taskGroup definition and makes it configurable, so the client can pass in the required values if needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#issuecomment-796569410
> Overall the change looks good, but I have one question: why we are not simply using the labels and annotations from the first real pod instead of asking the user to pass them in the taskGroup?
There could be a difference between the annotations for different taskgroups. We saw a sidecar injected on placeholders for one task group's placeholders which were blocked by an annotation on the real allocation. It only happened for 1 of the 2 task groups defined for the application.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#discussion_r592152660
##########
File path: go.sum
##########
@@ -829,6 +829,7 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
+golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
Review comment:
We should not update the go.sum in this change.
##########
File path: pkg/cache/placeholder.go
##########
@@ -73,6 +73,22 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
}
}
+// merge two string maps
+// if the same key defined in the first and second maps
+// the value will be set by the second map
+func mergeMaps(first, second map[string]string) map[string]string {
+ // add default labels first
Review comment:
This comment does not make sense: we're walking over a list of keys that could be anything, nothing to do with a label. Same for the comment below referencing taskgroup
##########
File path: pkg/cache/placeholder.go
##########
@@ -40,14 +40,14 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
ObjectMeta: metav1.ObjectMeta{
Name: placeholderName,
Namespace: app.tags[constants.AppTagNamespace],
- Labels: map[string]string{
+ Labels: mergeMaps(map[string]string{
constants.LabelApplicationID: app.GetApplicationID(),
constants.LabelQueueName: app.GetQueue(),
- },
- Annotations: map[string]string{
+ }, taskGroup.Labels),
Review comment:
The labels we set should not be overwritten by what the end user provides. We need it to be set to the correct app etc without allowing an override
##########
File path: pkg/cache/placeholder.go
##########
@@ -40,14 +40,14 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
ObjectMeta: metav1.ObjectMeta{
Name: placeholderName,
Namespace: app.tags[constants.AppTagNamespace],
- Labels: map[string]string{
+ Labels: mergeMaps(map[string]string{
constants.LabelApplicationID: app.GetApplicationID(),
constants.LabelQueueName: app.GetQueue(),
- },
- Annotations: map[string]string{
+ }, taskGroup.Labels),
+ Annotations: mergeMaps(map[string]string{
constants.AnnotationPlaceholderFlag: "true",
constants.AnnotationTaskGroupName: taskGroup.Name,
- },
+ }, taskGroup.Annotations),
Review comment:
I am not sure if we should override these values with the values from the annotations. We really need to make sure that these are set as we need and not allow overriding.
##########
File path: pkg/cache/placeholder_test.go
##########
@@ -68,6 +68,82 @@ func TestNewPlaceholder(t *testing.T) {
assert.Equal(t, holder.String(), "appID: app01, taskGroup: test-group-1, podName: test/ph-name")
}
+func TestNewPlaceholderWithLabelsAndAnnotations(t *testing.T) {
+ const (
+ appID = "app01"
+ queue = "root.default"
+ namespace = "test"
+ )
+ mockedSchedulerAPI := newMockSchedulerAPI()
+ app := NewApplication(appID, queue,
+ "bob", map[string]string{constants.AppTagNamespace: namespace}, mockedSchedulerAPI)
+ app.setTaskGroups([]v1alpha1.TaskGroup{
+ {
+ Name: "test-group-1",
+ MinMember: 10,
+ MinResource: map[string]resource.Quantity{
+ "cpu": resource.MustParse("500m"),
+ "memory": resource.MustParse("1024M"),
+ },
+ Labels: map[string]string{
+ "labelKey0" : "labelKeyValue0",
+ "labelKey1" : "labelKeyValue1",
+ },
+ Annotations: map[string]string{
+ "annotationKey0" : "annotationValue0",
+ "annotationKey1" : "annotationValue1",
+ "annotationKey2" : "annotationValue2",
+ },
+ },
+ })
+
+ holder := newPlaceholder("ph-name", app, app.taskGroups[0])
+ assert.Equal(t, len(holder.pod.Labels), 4)
+ assert.Equal(t, len(holder.pod.Annotations), 5)
+ assert.Equal(t, holder.pod.Labels["labelKey0"], "labelKeyValue0")
+ assert.Equal(t, holder.pod.Labels["labelKey1"], "labelKeyValue1")
+ assert.Equal(t, holder.pod.Annotations["annotationKey0"], "annotationValue0")
+ assert.Equal(t, holder.pod.Annotations["annotationKey1"], "annotationValue1")
+ assert.Equal(t, holder.pod.Annotations["annotationKey2"], "annotationValue2")
+}
+
+func TestMergeMaps(t *testing.T) {
+ result := mergeMaps(nil, nil)
+ assert.Assert(t, result != nil)
+ assert.Equal(t, len(result), 0)
Review comment:
two nil maps as input should return a nil map as output.
##########
File path: pkg/cache/placeholder.go
##########
@@ -73,6 +73,22 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
}
}
+// merge two string maps
+// if the same key defined in the first and second maps
+// the value will be set by the second map
Review comment:
This function should be defined as part of the `common.utils`. Easier for re-use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#discussion_r592183579
##########
File path: pkg/cache/placeholder.go
##########
@@ -73,6 +73,22 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
}
}
+// merge two string maps
+// if the same key defined in the first and second maps
+// the value will be set by the second map
+func mergeMaps(first, second map[string]string) map[string]string {
+ // add default labels first
Review comment:
That was leftover when I renaming the function, removed them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#discussion_r592236442
##########
File path: pkg/cache/placeholder_test.go
##########
@@ -68,6 +68,45 @@ func TestNewPlaceholder(t *testing.T) {
assert.Equal(t, holder.String(), "appID: app01, taskGroup: test-group-1, podName: test/ph-name")
}
+func TestNewPlaceholderWithLabelsAndAnnotations(t *testing.T) {
+ const (
+ appID = "app01"
+ queue = "root.default"
+ namespace = "test"
+ )
+ mockedSchedulerAPI := newMockSchedulerAPI()
+ app := NewApplication(appID, queue,
+ "bob", map[string]string{constants.AppTagNamespace: namespace}, mockedSchedulerAPI)
+ app.setTaskGroups([]v1alpha1.TaskGroup{
+ {
+ Name: "test-group-1",
+ MinMember: 10,
+ MinResource: map[string]resource.Quantity{
+ "cpu": resource.MustParse("500m"),
+ "memory": resource.MustParse("1024M"),
+ },
+ Labels: map[string]string{
+ "labelKey0": "labelKeyValue0",
+ "labelKey1": "labelKeyValue1",
+ },
+ Annotations: map[string]string{
+ "annotationKey0": "annotationValue0",
+ "annotationKey1": "annotationValue1",
+ "annotationKey2": "annotationValue2",
+ },
+ },
+ })
+
+ holder := newPlaceholder("ph-name", app, app.taskGroups[0])
+ assert.Equal(t, len(holder.pod.Labels), 4)
+ assert.Equal(t, len(holder.pod.Annotations), 5)
+ assert.Equal(t, holder.pod.Labels["labelKey0"], "labelKeyValue0")
+ assert.Equal(t, holder.pod.Labels["labelKey1"], "labelKeyValue1")
+ assert.Equal(t, holder.pod.Annotations["annotationKey0"], "annotationValue0")
+ assert.Equal(t, holder.pod.Annotations["annotationKey1"], "annotationValue1")
+ assert.Equal(t, holder.pod.Annotations["annotationKey2"], "annotationValue2")
Review comment:
nit: we can compare two maps in one line using assert.DeepEqual. With that we can make the comparison in 2 lines instead of 7
##########
File path: pkg/common/utils/utils_test.go
##########
@@ -331,3 +331,39 @@ func TestGetApplicationIDFromPod(t *testing.T) {
})
}
}
+
+func TestMergeMaps(t *testing.T) {
+ result := MergeMaps(nil, nil)
+ assert.Assert(t, result == nil)
+
+ result = MergeMaps(nil, map[string]string{"a": "b"})
+ assert.Assert(t, result != nil)
+ assert.Equal(t, len(result), 1)
+ assert.Equal(t, result["a"], "b")
+
+ result = MergeMaps(map[string]string{"a": "b"}, nil)
+ assert.Assert(t, result != nil)
+ assert.Equal(t, len(result), 1)
+ assert.Equal(t, result["a"], "b")
+
+ result = MergeMaps(map[string]string{"a": "a1"}, map[string]string{"a": "a2"})
+ assert.Assert(t, result != nil)
+ assert.Equal(t, len(result), 1)
+ assert.Equal(t, result["a"], "a2")
+
+ result = MergeMaps(map[string]string{
+ "a": "a1",
+ "b": "b1",
+ "c": "c1",
+ }, map[string]string{
+ "a": "a2",
+ "b": "b2",
+ "d": "d2",
+ })
+ assert.Assert(t, result != nil)
+ assert.Equal(t, len(result), 4)
+ assert.Equal(t, result["a"], "a2")
+ assert.Equal(t, result["b"], "b2")
+ assert.Equal(t, result["c"], "c1")
+ assert.Equal(t, result["d"], "d2")
+}
Review comment:
nit: if it is possible in go it is suggested to write table driven tests. It is more clear and elegant.
```
testCases := []struct {
name string
map1 map[string]string
map2 map[string]string
expectedResult map[string]string
}{
{"Nil maps", nil, nil, nil},
...... <create here the other cases>
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
assert.DeepEqual(t, MergeMaps(tc.map1, tc.map2), tc.expectedResult)
})
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#issuecomment-796592767
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239?src=pr&el=h1) Report
> Merging [#239](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239?src=pr&el=desc) (e29aa1b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.97%`.
> The diff coverage is `46.61%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #239 +/- ##
==========================================
- Coverage 59.75% 58.77% -0.98%
==========================================
Files 35 35
Lines 3133 3202 +69
==========================================
+ Hits 1872 1882 +10
- Misses 1180 1237 +57
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `70.40% <ø> (-4.00%)` | :arrow_down: |
| [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `43.33% <8.33%> (-9.73%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `72.57% <62.50%> (-4.17%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `63.15% <80.00%> (ø)` | |
| ... and [7 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239?src=pr&el=footer). Last update [8f15278...e29aa1b](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/239?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] kingamarton closed pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
kingamarton closed pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#issuecomment-796587480
Thanks for the quick review, @wilfred-s , @kingamarton pls check the latest changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#discussion_r592184225
##########
File path: pkg/cache/placeholder.go
##########
@@ -40,14 +40,14 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
ObjectMeta: metav1.ObjectMeta{
Name: placeholderName,
Namespace: app.tags[constants.AppTagNamespace],
- Labels: map[string]string{
+ Labels: mergeMaps(map[string]string{
constants.LabelApplicationID: app.GetApplicationID(),
constants.LabelQueueName: app.GetQueue(),
- },
- Annotations: map[string]string{
+ }, taskGroup.Labels),
+ Annotations: mergeMaps(map[string]string{
constants.AnnotationPlaceholderFlag: "true",
constants.AnnotationTaskGroupName: taskGroup.Name,
- },
+ }, taskGroup.Annotations),
Review comment:
good point, these values are needed, we should not allow overwrite.
I've switched around the argument to make sure they are always set.
##########
File path: pkg/cache/placeholder.go
##########
@@ -40,14 +40,14 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
ObjectMeta: metav1.ObjectMeta{
Name: placeholderName,
Namespace: app.tags[constants.AppTagNamespace],
- Labels: map[string]string{
+ Labels: mergeMaps(map[string]string{
constants.LabelApplicationID: app.GetApplicationID(),
constants.LabelQueueName: app.GetQueue(),
- },
- Annotations: map[string]string{
+ }, taskGroup.Labels),
Review comment:
Fixed
##########
File path: pkg/cache/placeholder_test.go
##########
@@ -68,6 +68,82 @@ func TestNewPlaceholder(t *testing.T) {
assert.Equal(t, holder.String(), "appID: app01, taskGroup: test-group-1, podName: test/ph-name")
}
+func TestNewPlaceholderWithLabelsAndAnnotations(t *testing.T) {
+ const (
+ appID = "app01"
+ queue = "root.default"
+ namespace = "test"
+ )
+ mockedSchedulerAPI := newMockSchedulerAPI()
+ app := NewApplication(appID, queue,
+ "bob", map[string]string{constants.AppTagNamespace: namespace}, mockedSchedulerAPI)
+ app.setTaskGroups([]v1alpha1.TaskGroup{
+ {
+ Name: "test-group-1",
+ MinMember: 10,
+ MinResource: map[string]resource.Quantity{
+ "cpu": resource.MustParse("500m"),
+ "memory": resource.MustParse("1024M"),
+ },
+ Labels: map[string]string{
+ "labelKey0" : "labelKeyValue0",
+ "labelKey1" : "labelKeyValue1",
+ },
+ Annotations: map[string]string{
+ "annotationKey0" : "annotationValue0",
+ "annotationKey1" : "annotationValue1",
+ "annotationKey2" : "annotationValue2",
+ },
+ },
+ })
+
+ holder := newPlaceholder("ph-name", app, app.taskGroups[0])
+ assert.Equal(t, len(holder.pod.Labels), 4)
+ assert.Equal(t, len(holder.pod.Annotations), 5)
+ assert.Equal(t, holder.pod.Labels["labelKey0"], "labelKeyValue0")
+ assert.Equal(t, holder.pod.Labels["labelKey1"], "labelKeyValue1")
+ assert.Equal(t, holder.pod.Annotations["annotationKey0"], "annotationValue0")
+ assert.Equal(t, holder.pod.Annotations["annotationKey1"], "annotationValue1")
+ assert.Equal(t, holder.pod.Annotations["annotationKey2"], "annotationValue2")
+}
+
+func TestMergeMaps(t *testing.T) {
+ result := mergeMaps(nil, nil)
+ assert.Assert(t, result != nil)
+ assert.Equal(t, len(result), 0)
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#discussion_r592183765
##########
File path: pkg/cache/placeholder.go
##########
@@ -73,6 +73,22 @@ func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1
}
}
+// merge two string maps
+// if the same key defined in the first and second maps
+// the value will be set by the second map
Review comment:
Moved to util package for re-use
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #239: [YUNIKORN-562] Support add labels/annotations to taskGroup
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #239:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/239#discussion_r592183204
##########
File path: go.sum
##########
@@ -829,6 +829,7 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 h1:E7g+9GITq07hpfrRu66IVDexMakfv52eLZ2CXBWiKr4=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
+golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 h1:go1bK/D/BFZV2I8cIQd1NKEZ+0owSTG1fDTci4IqFcE=
Review comment:
Reverted
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org