-
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
New resource: azurerm_mssql_job_target_group
#28492
base: main
Are you sure you want to change the base?
Conversation
switch v.Type { | ||
case string(jobtargetgroups.JobTargetTypeSqlDatabase): | ||
if v.DatabaseName == "" { | ||
return nil, fmt.Errorf("`database_name` is required when `type` is `%s`", jobtargetgroups.JobTargetTypeSqlDatabase) | ||
} | ||
|
||
t.DatabaseName = pointer.To(v.DatabaseName) | ||
case string(jobtargetgroups.JobTargetTypeSqlElasticPool): | ||
if v.ElasticPoolName == "" { | ||
return nil, fmt.Errorf("`elastic_pool_name` is required when `type` is `%s`", jobtargetgroups.JobTargetTypeSqlElasticPool) | ||
} | ||
|
||
t.ElasticPoolName = pointer.To(v.ElasticPoolName) | ||
} |
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.
Based on the logic here I'm wondering whether it makes sense to not expose the type
property and to hardcode it for users e.g.
- If only
server_name
is supplied, type is set to server - If
server_name
anddatabase_name
are supplied, we set type to database - If
server_name
andelastic_pool_name
are supplied, we set type to elastic pool
Might make the validation on line 278-284 tricky, but just a thought.
Furthermore I think there is some validation here that we can move into the schema or add to prevent misconfiguration:
- Only one of
database_name
orelastic_pool_name
can be specified (not both together) so we may want to add theConflictsWith
attribute on those in the schema - It appears
job_credential_id
is required if the type is either server or elastic pool, so we could take advantage of theRequiredWith
attribute (though this one might not be as simple as I initially thought given it also depends on the value ofmembership_type
🙈)
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.
As discussed offline, unfortunately I can't use ConflictsWith
and RequiredWith
, though I have moved the validation into a CustomizeDiff
so it's caught during the plan.
The job_target.type
argument is now set by the provider and is no longer a configurable argument.
internal/services/mssql/mssql_job_target_group_resource_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: stephybun <[email protected]>
Community Note
Description
Added new resource
azurerm_mssql_job_target_group
E: will resolve conflicts after review
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_mssql_job_target_group
[New resource:azurerm_mssql_job_target_group
#28492]This is a (please select all that apply):
Note
If this PR changes meaningfully during the course of review please update the title and description as required.