Drop duplicate instance tags, if exists #78
Conversation
|
|
||
| // look for duplicates | ||
| for i := range tags { | ||
| if m[*tags[i].Key] == true { |
There was a problem hiding this comment.
This, I think, can be reduced to:
...
keys := make(map[string]bool)
result := []*ec2.Tag{}
for _, entry := range tags {
if _, value := keys[*entry.Key]; !value {
keys[*entry.Key] = true
result = append(result, entry)
}
}
return result
}
but untested - I just typed it out!
Please could you also create a unit test for this function as it it generic.
|
|
||
| // look for duplicates | ||
| for i := range tags { | ||
| if m[*tags[i].Key] == true { |
There was a problem hiding this comment.
This, I think, can be reduced to:
...
keys := make(map[string]bool)
result := []*ec2.Tag{}
for _, entry := range tags {
if _, value := keys[*entry.Key]; !value {
keys[*entry.Key] = true
result = append(result, entry)
}
}
return result
}
but untested - I just typed it out!
Please could you also create a unit test for this function as it it generic.
|
/test e2e-aws |
| } | ||
|
|
||
| for i, c := range cases { | ||
| if len(c.deduplicatedList) != len(removeDuplicatedTags(c.tagList)) { |
There was a problem hiding this comment.
The length doesn't guarantee correctness. Better to have expected and actual and use reflect.DeepEquals to verify the result.
| }{ | ||
| { | ||
| // no duplicate tags | ||
| tagList: []*ec2.Tag{ |
There was a problem hiding this comment.
Add the empty case (i.e., empty slices).
| for i, c := range cases { | ||
| actual := removeDuplicatedTags(c.tagList) | ||
| if !reflect.DeepEqual(c.expected, actual) { | ||
| t.Errorf("test case %d: output of removeDuplicatedTags is not as expected.\n", i) |
There was a problem hiding this comment.
I think it would be more succinct to reduce all of this output to just:
t.Errorf("test #%d: expected %+v, got %+v", c.expected, actual)
as the inputs are quite small.
| } | ||
| tagList = append(tagList, []*ec2.Tag{ | ||
| rawTagList = append(rawTagList, []*ec2.Tag{ |
There was a problem hiding this comment.
Can't we just put all the tags under a map and then check if the key already exists?
tagMap := make(map[string]struct{})
for _, tag := range machineProviderConfig.Tags {
tagMap[tag.Name] = struct{}{}
}
if _, exists := tagMap["clusterid"]; !exists {
tagList = append(tagList, {Key: aws.String("clusterid"), Value: aws.String(clusterID)}}
}
...There was a problem hiding this comment.
personally I like current implementation. It prevents also from injecting duplications @frobware wdyt?
There was a problem hiding this comment.
I kind of prefer the de-dupe phase. It's easier to rationalise. I have a bunch of tags, then a last action is to de-dupe them.
|
/lgtm |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
related to: openshift/installer#468 (comment)