-
Notifications
You must be signed in to change notification settings - Fork 204
feat: Add azure attributes in federated database instance resource #3484
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
base: master
Are you sure you want to change the base?
feat: Add azure attributes in federated database instance resource #3484
Conversation
…_database_instance
test: add Azure cloud provider acceptance test for federated database resource
…est functionality on Azure Cloud Provider Missing refactor
@@ -63,7 +63,7 @@ func Resource() *schema.Resource { | |||
"aws": { | |||
Type: schema.TypeList, | |||
MaxItems: 1, | |||
Required: true, | |||
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.
Investigation notes:
cloud_provider_config
(parent attribute) is not required in the create operation and is not returned from the API when it is not set. However, the attribute is already marked as Optional+Computed, so keeping it as is to allow no-op when the attribute is removed.aws
changing fromrequired
tooptional
to allowazure
usage
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.
Addressed in d7da829ac0db2f95da04417bce4fb47c9c5eb1c6
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.
This information: Changing from required to optional to allow azure usage
I would keep only as a PR comment.
Later after this is merged it will be no surprise to find this as optional.
internal/service/federateddatabaseinstance/resource_federated_database_instance_test.go
Outdated
Show resolved
Hide resolved
internal/service/federateddatabaseinstance/resource_federated_database_instance_test.go
Outdated
Show resolved
Hide resolved
internal/service/federateddatabaseinstance/resource_federated_database_instance.go
Outdated
Show resolved
Hide resolved
return nil | ||
} | ||
|
||
// test_s3_bucket is not part of the API response |
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.
Looks like we might need to pass the d *schema.ResourceData
as before, as the tests are showing that this comment is still valid :/
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 need to add:
AZURE_ATLAS_APP_ID: ${{ inputs.azure_atlas_app_id }}
AZURE_SERVICE_PRINCIPAL_ID: ${{ inputs.azure_service_principal_id }}
AZURE_TENANT_ID: ${{ inputs.azure_tenant_id }}
Here: MONGODB_ATLAS_FEDERATION_SETTINGS_ID: ${{ inputs.mongodb_atlas_federation_settings_id }} so the test can use Azure in CI
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.
Looks like we need to update the schema of the data sources too. Due to this failing tests:
https://github.com/mongodb/terraform-provider-mongodbatlas/actions/runs/16174869633/job/45657910272#step:5:91
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*ResourceData).Set(0xc001192750, {0x2d7628f, 0x15}, {0x2619600, 0xc001a8eff0})
/home/runner/go/pkg/mod/github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource_data.go:238 +0x2b2
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federateddatabaseinstance.dataSourceMongoDBAtlasFederatedDatabaseInstanceRead({0x3197340, 0xc00057e150}, 0xc001192750, {0x2864b80?, 0xc001b6d740?})
/home/runner/work/terraform-provider-mongodbatlas/terraform-provider-mongodbatlas/internal/service/federateddatabaseinstance/data_source_federated_database_instance.go:336 +0x78d
"azure": { | ||
Type: schema.TypeList, | ||
MaxItems: 1, | ||
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.
All the nested fields starting from cloud_provider_config
can be Computed
only.
Also, please refactor out the schema for cloud_provider_config
and use it in the plural data source too.
Bonus, you can even use it for the resource, with an isDatasource
argument
@@ -53,6 +53,7 @@ func Resource() *schema.Resource { | |||
Type: schema.TypeString, | |||
}, | |||
}, | |||
// attribute not required in the create operation and not returned from the API when it is not set. |
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.
[nit] Start with uppercase
[nit] A lot of not
, maybe it can be written clearer?
For example, Optional-only behavior from the API, but keeping O+C to avoid behavior changes.
…o its own function to be able to reuse it in multiple places
This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
Description
federated_database_instance
acc.cloud_provider_access.go
Link to any related issue(s): CLOUDP-245406
Follow up Work
Type of change:
Required Checklist: