-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_datadog_monitor_sso_configuration
deprecate single_sign_on_enabled
property
#28520
base: main
Are you sure you want to change the base?
Conversation
internal/services/kusto/kusto_attached_database_configuration_resource.go
Outdated
Show resolved
Hide resolved
internal/services/kusto/kusto_eventgrid_data_connection_resource.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @wyattfry. Datadog SSO stuff looks great as does removing the provider test stuff but unfortunately, some of the 4.0 removal stuff can't be done because it currently works in 4.0 and will break people if we remove it the way it's written
internal/services/datadog/azurerm_datadog_monitor_sso_configuration_test.go
Outdated
Show resolved
Hide resolved
internal/services/servicebus/servicebus_subscription_data_source.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spotted a couple extras to @mbfrahry's review
internal/services/datadog/azurerm_datadog_monitor_sso_configuration.go
Outdated
Show resolved
Hide resolved
internal/services/datadog/azurerm_datadog_monitor_sso_configuration.go
Outdated
Show resolved
Hide resolved
…r-azurerm into wyatt/todos-4-0-datadog-sso-config
I've discovered that i am able to make new datadog instances via azure portal (in azure terms, a "datadog monitor", not be confused with monitors within datadog), and do not need hashicorp to provision a datadog organization in order to test this resource. To progress this work, whoever takes it, be it myself or anyone else, they should be able to progress with the self-serve datadog instances: ![]() |
Closes this PR until I have a chance to focus on it. |
…r-azurerm into wyatt/todos-4-0-datadog-sso-config
…r-azurerm into wyatt/todos-4-0-datadog-sso-config
single_sign_on_enabled = "Enable" | ||
enterprise_application_id = "XXXX" | ||
single_sign_on = "Enable" | ||
enterprise_application_id = "00000000-0000-0000-0000-000000000000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because enterprise app IDs are always GUIDs, I felt it would be clearer to make the example value a GUID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @wyattfry, I've left some comments on this review. Some of which are covered under the breaking-changes guide which I encourage you to look over
Type: pluginsdk.TypeString, | ||
Required: true, | ||
ValidateFunc: validation.StringInSlice([]string{ | ||
// @tombuildsstuff: other options are available, but the Create handles this as a boolean for now | ||
// should the field be a boolean? one to consider for 4.0 when this resource is inlined | ||
string(singlesignon.SingleSignOnStatesEnable), | ||
string(singlesignon.SingleSignOnStatesDisable), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add support for the other values here as well? From what we saw, I doubt we'll need them but we may as well let people use them if they want
|
||
if !features.FivePointOh() { | ||
resource.Schema["single_sign_on"].Required = false | ||
resource.Schema["single_sign_on"].Optional = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want these to be computed as well since we're setting both into state even though the user will only specify one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, at this point, we could just have rewrite this with another block instead of changing the existing block
resource.Schema["single_sign_on"] = &pluginsdk.Schema{
...
}
resource.Schema["single_sign_on"].Optional = true | ||
resource.Schema["single_sign_on_enabled"] = &pluginsdk.Schema{ | ||
Type: pluginsdk.TypeString, | ||
Optional: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And computed here
internal/services/datadog/azurerm_datadog_monitor_sso_configuration.go
Outdated
Show resolved
Hide resolved
internal/services/datadog/azurerm_datadog_monitor_sso_configuration.go
Outdated
Show resolved
Hide resolved
}, | ||
} | ||
|
||
if v, ok := d.GetOk("single_sign_on"); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should refactor this update method to use d.HasChanges
so people can use ignore_changes
if they want
@@ -50,7 +50,7 @@ The following arguments are supported: | |||
|
|||
* `datadog_monitor_id` - (Required) The Datadog Monitor Id which should be used for this Datadog Monitor SSO Configuration. Changing this forces a new Datadog Monitor SSO Configuration to be created. | |||
|
|||
* `single_sign_on_enabled` - (Required) The state of SingleSignOn configuration. Possible values are `Enable` and `Disable`. | |||
* `single_sign_on` - (Required) The state of SingleSignOn configuration. Possible values are `Enable`, `Disable`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll also want to add a note in the 5.0 upgrade guide
@@ -64,7 +64,6 @@ func TestAccDatadogMonitorSSO_requiresImport(t *testing.T) { | |||
Config: r.basic(data), | |||
Check: acceptance.ComposeTestCheckFunc( | |||
check.That(data.ResourceName).ExistsInAzure(r), | |||
check.That(data.ResourceName).Key("single_sign_on_enabled").HasValue("Enable"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to update the tests to use the new value and keep a legacy test config around
internal/services/datadog/azurerm_datadog_monitor_sso_configuration.go
Outdated
Show resolved
Hide resolved
…r-azurerm into wyatt/todos-4-0-datadog-sso-config
Co-authored-by: Matthew Frahry <[email protected]>
…om/hashicorp/terraform-provider-azurerm into wyatt/todos-4-0-datadog-sso-config
Community Note
Description
single_sign_on_enabled
property in favor ofsingle_sign_on
(a 4.0 TODO)PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
The unit tests have been skipped for a long time and fixing them is outside the scope of this PR.
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_datadog_monitor_sso_configuration
replacesingle_sign_on_enabled
withsingle_sign_on
property [azurerm_datadog_monitor_sso_configuration
deprecatesingle_sign_on_enabled
property #28520]This is a (please select all that apply):
Related Issue(s)
Relates to https://github.com/hashicorp/tf-azure-stratosphere/issues/245
Note
If this PR changes meaningfully during the course of review please update the title and description as required.