Tag and Untag API for cluster. #43
Conversation
kyriechen96
force-pushed
the
clusterTagging
branch
from
a40d55f to
69d6858
Compare
| ResourceArn: &resourceARN, | ||
| }, | ||
| ) | ||
| rm.metrics.RecordAPICall("GET", "ListTags", err) |
There was a problem hiding this comment.
This should be a READ_MANY instead of GET
There was a problem hiding this comment.
Thank you for correcting me!
There was a problem hiding this comment.
After careful consideration, I still think it should be GET here. May I know why you think it should be READ_MANY here instead? @jljaco
There was a problem hiding this comment.
@kyriechen96 please disregard this comment -- there may be some inconsistencies across ACK controllers in how this choice is made, and we will circle back to this later if necessary. you're good with GET!
pkg/resource/cluster/hooks.go
Outdated
| ) (err error) { | ||
| rlog := ackrtlog.FromContext(ctx) | ||
| exit := rlog.Trace("rm.syncTags") | ||
| defer func() { exit(err) }() |
There was a problem hiding this comment.
defer func(err error) { exit(err) }(err)
pkg/resource/cluster/hooks.go
Outdated
| } | ||
|
|
||
| for _, tag := range desired { | ||
| toAdd = append(toAdd, tag) |
There was a problem hiding this comment.
Do you want to check whether tag is in latest before putting it in the toAdd list?
pkg/resource/cluster/hooks.go
Outdated
| latest *resource, | ||
| ) (err error) { | ||
| rlog := ackrtlog.FromContext(ctx) | ||
| exit := rlog.Trace("rm.syncTags") |
There was a problem hiding this comment.
this should match the function name; so either rename the function syncTags or change this to rm.updateTags
There was a problem hiding this comment.
You're right. Forgot changing it.
pkg/resource/cluster/hooks.go
Outdated
| return nil, err | ||
| } | ||
| tags := make([]*svcapitypes.Tag, 0, len(resp.TagList)) | ||
| for _, tag := range resp.TagList { |
There was a problem hiding this comment.
You've implemented sdkTagsFromResourceTags below; consider adding a function resourceTagsFromSDKTags to do this, which is the inverse transformation.
|
|
||
| resourceARN := (*string)(ko.Status.ACKResourceMetadata.ARN) | ||
| tags, err := rm.getTags(ctx, *resourceARN) |
There was a problem hiding this comment.
Not sure why this happened. The indentation looks good in my local file.
There was a problem hiding this comment.
Github bug I think
kyriechen96
force-pushed
the
clusterTagging
branch
from
69d6858 to
f419524
Compare
kyriechen96
force-pushed
the
clusterTagging
branch
from
f419524 to
c263865
Compare
pkg/resource/cluster/hooks.go
Outdated
| "errors" | ||
| "fmt" | ||
| ackutil "github.com/aws-controllers-k8s/runtime/pkg/util" | ||
|
|
||
| ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare" | ||
| ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors" | ||
| ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log" | ||
| "strconv" |
pkg/resource/cluster/hooks.go
Outdated
| ) | ||
|
|
||
| if len(toDelete) > 0 { | ||
| rlog.Debug("removing tags from parameter group", "tags", toDelete) |
pkg/resource/cluster/hooks.go
Outdated
| } | ||
|
|
||
| if len(toAdd) > 0 { | ||
| rlog.Debug("adding tags to parameter group", "tags", toAdd) |
kyriechen96
force-pushed
the
clusterTagging
branch
from
c263865 to
ebcc270
Compare
|
@jljaco and I discussed about the READ_MANY vs GET issue. For now we don't have a precise answer and we'll try to discuss tomorrow. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, kyriechen96 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 |
Issue #, if available:
ACK MemoryDB controller doesn't provide tags update after creation of cluster.
Description of changes:
Add functions so that customers can update tags of cluster after creation.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.