feat: Add contributorInsights field to Table resource #127
Conversation
pkg/resource/table/hooks.go
Outdated
| @@ -590,6 +600,10 @@ func customPreCompare( | |||
| if a.ko.Spec.DeletionProtectionEnabled == nil { | |||
| a.ko.Spec.DeletionProtectionEnabled = aws.Bool(false) | |||
| } | |||
|
|
|||
| if a.ko.Spec.ContributorInsights == nil && *b.ko.Spec.ContributorInsights == string(svcsdktypes.ContributorInsightsStatusDisabled) { | |||
There was a problem hiding this comment.
b.ko.Spec.ContributorInsights is always non nil? would something like this work?
if a.ko.Spec.ContributorInsights == nil {
b.ko.Spec.ContributorInsights = a.ko.Spec.ContributorInsights
}There was a problem hiding this comment.
so the default value of this field is DISABLED..
if the user completely removes this field, and the insight is ENABLED, i thought we could disable it...
There was a problem hiding this comment.
QQ: Should the ContributorInsights field accept ENABLED/DISABLED? or ENABLE/DISABLE?
i realized the output of the describe is different than the update after i generated..what do you think?
| case svcsdktypes.ContributorInsightsStatusDisabled: | ||
| ko.Spec.ContributorInsights = aws.String(string(svcsdktypes.ContributorInsightsActionDisable)) | ||
| default: | ||
| ko.Spec.ContributorInsights = aws.String(string(resp.ContributorInsightsStatus)) |
There was a problem hiding this comment.
how about just ko.Spec.ContributorInsights = aws.String(string(resp.ContributorInsightsStatus)) are the other cases necessary?
There was a problem hiding this comment.
describe returns: enabled, disabled, enabling, disabling
update accepts: enable, disable
converting them here makes the delta easier...
There was a problem hiding this comment.
Shouldn't we just requeue if it's in a transition state and assume that if we reach this block of code it's either DISABLED or ENABLED?
There was a problem hiding this comment.
the requeue is happening at the end of sdkFind..inside isTableContributorInsightsUpdating that i'm adding
michaelhtm
force-pushed
the
feat/insights
branch
from
ac13306 to
0ea8584
Compare
michaelhtm
force-pushed
the
feat/insights
branch
from
0ea8584 to
ac49864
Compare
|
@michaelhtm: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, michaelhtm, rushmash91 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 #2408
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.