search

splunk / terraform-provider-splunk

Terraform Provider for Splunk
Mozilla Public License 2.0
103 stars 76 forks source link

#76 Use Defaults for authorization/roles that will have defaults applied by Splunk anyway. #77

Closed micahkemp-splunk closed 3 years ago

micahkemp-splunk commented 3 years ago

By setting these defaults we can remove "omitempty" for these values, enabling the setting of explicit zero values.

micahkemp-splunk commented 3 years ago

Fixes #76 (and #73, which seems identical).

anushjay commented 3 years ago

@micahkemp-splunk Ideally, we don't want to have terraform to set default values. But I understand that the SDK doesn't give another option. Additionally, I think changing defaults requires a state migration function as well?

micahkemp-splunk commented 3 years ago

Additionally, I think changing defaults requires a state migration function as well?

Looking at what little documentation I can find regarding state migration functions, I'm not sure this needs one. The example shows this:

func resourceExampleInstanceStateUpgradeV0(rawState map[string]interface{}, meta interface{}) (map[string]interface{}, error) {
    rawState["name"] = rawState["name"] + "."

    return rawState, nil
}

It seems to me that this is only needed if the previous state would be invalid for the new schema version. Or perhaps the schema version needs to be bumped and the migration function needs to be defined to just make no changes.

The reason being it doesn't seem to me that the stored state would need to change at all for the default values to be changed. Those defaults are already in state, because the REST API returned them during Read already.

But I've never done a state migration before, so I'm not entirely sure if changing defaults necessitates a schema version bump at all. I think I'll need to leave that decision up to the maintainers, but if you inform me of your decision regarding needing one or not, I'll update the PR as needed to meet your requirements.

anushjay commented 3 years ago

@micahkemp-splunk I agree about state migration not needed in this case. I tried to manually test the changes by building the provider with your changes. The refresh doesn't change anything in the state files and any updates to the fields are working as expected. I'm gonna merge your PR and create a release. I'll keep an eye out for any issues regarding this change.