r/redshift_cluster: Nest all logging fields #2230
Conversation
radeksimko
added
the
enhancement
radeksimko
force-pushed
the
f-redshift-reshuffle-schema
branch
from
9bde399
to
be5f98f
Compare
| Optional: true, | ||
| Computed: true, | ||
| Deprecated: "Use enable in the 'logging' block instead", | ||
| ConflictsWith: []string{"logging"}, |
There was a problem hiding this comment.
would it make more sense to make this logging.0.enabled?
There was a problem hiding this comment.
I think there's no harm in conflicting with the whole logging block. Either they use deprecated field or the whole new block. I'd rather not deal with mix of deprecated and non-deprecated fields. 😉
| logging, ok := d.GetOk("logging.0.enable") | ||
| _, deprecatedOk := d.GetOk("enable_logging") | ||
|
|
||
| if (ok && logging.(bool)) || deprecatedOk { |
There was a problem hiding this comment.
I think you want to check the value of enable_logging, rather than that it's set to a value?
There was a problem hiding this comment.
As it is a boolean field it shouldn't matter, b/c it will always be present - that is in fact why we introduced d.GetOkExists, secondly I intentionally left the implementation as is since it will be deprecated eventually.
| Schema: map[string]*schema.Schema{ | ||
| "enable": { | ||
| Type: schema.TypeBool, | ||
| Required: true, |
There was a problem hiding this comment.
does this not need a State Migration?
There was a problem hiding this comment.
I don't think so - it's only required within the context of logging block, i.e. you can't define an empty logging {}, it shouldn't affect any existing configs without this block.
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
This is to make the schema aligned more with the API and also to leave some name space for future new fields.
Test Results