Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

micahkemp-splunk
Copy link
Contributor

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

…by Splunk anyway. By setting these defaults we can remove "omitempty" for these values, enabling the setting of explicit zero values.
@micahkemp-splunk
Copy link
Contributor Author

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

@anushjay
Copy link
Collaborator

@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
Copy link
Contributor Author

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
Copy link
Collaborator

@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.

@anushjay anushjay merged commit 714f7b1 into splunk:master Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants