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

set default value for use_versioned_api param to true #555

Merged
merged 3 commits into from
Aug 3, 2023

Conversation

vsinghal13
Copy link
Collaborator

@vsinghal13 vsinghal13 commented Jul 28, 2023

Currently, we do not have any default value for use_versioned_api parameter. The default value for the optional boolean in goLang is false.

Golang bool does not have a way to identify if the value was explicitly set or not . It is not able to identify the difference between if the value is set to false or the parameter is not set thereby taking default value as false.

When users are setting the parameter to false, it is not being passed to backend and therefore the resource is being created with use_versioned_api as true despite the config block having false.

What we want is:

  • if user specifies true => true
  • if user specifies false => false
  • if user does not specify => true (backward compatibility)

This PR converts the parameter to use a *bool and sets the default value as true and we are able to achieve the above behavior except for one caveat. Since the parameter has a default value of true it gets passed in other path types as well, eg. CloudWatchPath, where this parameter is not actionable, resulting in showing that the value needs to be changed when running terraform plan every time. Therefore, we have added a custom Diff method to suppress them when needed.

Testing performed:

  • able to create a source with true and false values for use_versioned_api.
  • able to create a source without any value for use_versioned_api and it takes true as default.
  • verified ignore_changes works for other S3-related resources to ignore any changes to this parameter.

@vsinghal13 vsinghal13 force-pushed the vsinghal-fix-s3-use-versioned-api branch from 4e17ca6 to 19d6fca Compare July 28, 2023 12:19
@sumovishal
Copy link
Collaborator

For this, we can add the block in the tf resource config

lifecycle {
ignore_changes = [
path["use_versioned_api"],
]
}

I don't think this is the right approach. This will not be a good experience for customers. Can we write a custom diff method to suppress diff in some cases?

@vsinghal13
Copy link
Collaborator Author

@sumovishal Ideally we should not pass the value in cases where we don't want it. That is why we are using a bool pointer to have a tristate boolean property. I tried setting pathSettings.UseVersionedApi = nil to skip that parameter from being passed, but it's not honoring that and still passing the default value. Therefore, had to go this route.

@vsinghal13 vsinghal13 force-pushed the vsinghal-fix-s3-use-versioned-api branch 2 times, most recently from 4f05c18 to 241d7ba Compare August 2, 2023 06:10
@vsinghal13 vsinghal13 force-pushed the vsinghal-fix-s3-use-versioned-api branch from 241d7ba to cbc2c78 Compare August 2, 2023 06:10
@vsinghal13
Copy link
Collaborator Author

@sumovishal I have added the custom diff method to suppress changes when needed.

@sumovishal
Copy link
Collaborator

@sumovishal I have added the custom diff method to suppress changes when needed.

thanks, can you get it reviewed by someone from the Collection team?

@vsinghal13
Copy link
Collaborator Author

@sumovishal could you please force merge this as unrelated tests are failing?

@sumovishal sumovishal merged commit 48bb426 into master Aug 3, 2023
2 of 3 checks passed
@sumovishal sumovishal deleted the vsinghal-fix-s3-use-versioned-api branch August 3, 2023 13:51
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.

4 participants