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

azurerm_mssql_server changes minimum_tls_version value from "Disabled" to "None" #21896

Closed

Conversation

Pidgegeddon
Copy link

It looks like Azure have changed the value for minimum_tls_version from "Disabled" to "None" in the API.
The function msSqlMinimumTLSVersionDiff checks for the value "Disabled" and thus always throws an error now.

This is my first time with go and contributing here so if there is a better way to do this than please advise 🙂

fixes #21882

@Pidgegeddon Pidgegeddon marked this pull request as ready for review May 23, 2023 09:19
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pr @Pidgegeddon, however this is a breaking change and doesn't need to be. we can either support both so it doesn't break users config or we can salinity convert the properties under the hood (and either way remove the old value in 4.0)

once thats done this should be good to merge!

internal/services/mssql/mssql_server_resource.go Outdated Show resolved Hide resolved
internal/services/mssql/mssql_server_resource.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Pidgegeddon ! LGTM 🦕

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Pidgegeddon, thanks for taking the time to open this PR! It looks like this is a breaking api change as, like you noted, Disabled is now being returned as None. I'm not sure if this is intended on their end but we've got an open issue on their api-specs to confirm.

As for this PR, we're going to close it down as we have a temporary solution with #22067 that should tide us over until we get confirmation on what the api is doing and whether None is needed as both None and Disabled seem to do the same thing

@mbfrahry mbfrahry closed this Jun 8, 2023
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
3 participants