INTMDB-802: Create new TF Data Federation Query Limit resource and data sources #1173

maastha · 2023-05-17T11:03:26Z

Description

Added support for Data Federation Query limit [Resource and Data source]

Jira Ticket: INTMDB-802

Link to any related issue(s):

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contribution guidelines
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code

Further comments

… Limit

maastha · 2023-05-25T08:17:44Z

Example check currently failing because some deprecated resources have not been removed. Fixed in
1181 but yet to be merged.

examples/federated-database-query-limit/README.md

andreaangiolillo · 2023-05-30T10:17:34Z

examples/federated-database-query-limit/main.tf

+  aws {
+    role_id        = ""
+    test_s3_bucket = ""
+  }


Shoun't we use variables here?

This example doesn't use AWS S3, it creates federated instance based on two Atlas clusters

AWS is only required if you are federating with an S3 data source so these shouldn't be in the sample if an S3 bucket isn't being used.

aws is a required field in the federated instance resource. @themantissa we will need to make this an optional field first in this case. Please confirm.
cc @andreaangiolillo

believe we are talking about 2 different parameters in this endpoint. under cloudProviderConfig we have aws being a required parameter which has roleId and testS3Bucket as sub-parameters so overall i think @maastha is correct in this example.

further down there is a stores parameter which has a required sub-parameter for provider of which users can select between: atlas, http, online_archive, or S3. in this example we are selecting atlas provider under storage_storage few lines below so additional details (i.e. S3 Bucket Region) aren't needed here.

No concerns, thanks both @maastha and @andreaangiolillo. just ping me when this PR has been updated and happy to take another review.

@andreaangiolillo @Zuhairahmed I do have some concerns - I'm assuming this is in a cloudProviderConfig block because eventually it would have other cloud providers beyond AWS, e.g. Azure, GCP, for their object storage. We can certainly solve this just to make each, in turn, optional in Terraform and if we want to move faster that's a way we can choose to go. But I think this makes for harder to read HCL code - it means a dangling cloud block that even we had a hard time figuring out why it's there. If we end up with 3 cloud providers we'll have 3 of these. Really part of the issue here is naming. I think it would have been better for the API parameter to have a name like cloudProviderObjectStorageConfig but only @baf509 and team can explain why the shorter but harder to figure out name. So I think we should note in the docs that aws{} is required if using object storage and at least separate it in Terraform in some way like:

cloudProviderConfig {
aws {
role_id = ""
test_s3_bucket = ""
}
//to make space for when we eventually have:
azure {
//required variables if using object storage
}
gcp {
//required variables if using object storage
}
}

It's good feedback. Current approach should be fine in short term, but as soon as we get support Azure or Google Cloud here it will get confusing as you point out @themantissa. I also think this is a matter of when, not if so suggest we invest the time this time around to make clear from the start.

@andreaangiolillo any concerns adding back in cloudProviderConfig in doc fix per above in mongodbatlas_federated_database_instance? If you are also aligned would also then to make minor commits in this PR to adjust examples accordingly here as well.

no concerns. I am updating mongodbatlas_federated_database_instance and documentation in #1215

@Zuhairahmed @themantissa updated. Thanks!

go.mod

go.sum

mongodbatlas/data_source_mongodbatlas_federated_query_limit.go

mongodbatlas/resource_mongodbatlas_federated_query_limit.go

Co-authored-by: Andrea Angiolillo <[email protected]>

themantissa

I think this question https://github.com/mongodb/terraform-provider-mongodbatlas/pull/1173/files#r1210058761 is surfacing something important we should dig into before proceeding. cc @andreaangiolillo

website/docs/r/federated_query_limit.html.markdown

themantissa · 2023-05-31T16:28:35Z

examples/federated-database-query-limit/main.tf

+  aws {
+    role_id        = ""
+    test_s3_bucket = ""
+  }


So let's take a step back first and cover why both @andreaangiolillo and I are commenting on this. We should not have empty parameters like this. We've never had it yet so we need to figure out why and if they are needed.

To Zuhair's point - he's correct. There are two AWS areas. I thought this referred to the stores at first glance but he's correct, it doesn't. It looks like a holding place for when this supports more than one cloud provider. Right now it's just AWS and the docs note it's only "Required if specifying cloudProviderConfig". I would think you'd only need to specify this when it moves beyond a single cloud provider. Really this should be within a cloudProviderConfig block so perhaps we need to revisit the PR there (https://github.com/mongodb/terraform-provider-mongodbatlas/pull/1163/files#diff-ebd5e08c7fa8e4f0fee0f3b1c8c687c52fd98ce77434a1053e591ee3952d7902R69). Furthermore, I don't see how a user would need to provide this- wouldn't that be a MDB S3 bucket this federation is stored in? Do we store it in the users bucket and if so this would likely need data from cloud provider access - which means we need a better example of that. Can we perhaps reach out to the Data Federation team to get their help there?

martinstibbe

LGTM

zach-carr

LGTM except for what I think is a typo (First Atlas Cluster Name), left 1 suggestion

examples/federated-database-query-limit/README.md

examples/federated-database-query-limit/variables.tf

Co-authored-by: zach-carr <[email protected]>

…urces only (not resource) (#1177)

…ta sources which supports Federated Database Instance and Online Archive (#1182)

maastha · 2023-06-02T17:05:19Z

@Zuhairahmed @andreaangiolillo Updated this PR with required changes.

@andreaangiolillo Please feel free to merge if all looks good.

Zuhairahmed

LGTM!

…ngodbatlas into INTMDB-802

andreaangiolillo · 2023-06-05T09:13:10Z

@Zuhairahmed Could you confirm that we can merge this PR without waiting for Melissa? Thanks

Zuhairahmed · 2023-06-05T14:24:21Z

@Zuhairahmed Could you confirm that we can merge this PR without waiting for Melissa? Thanks

@andreaangiolillo suggest let's hold a bit, @themantissa should be able to close out this review tomorrow

colm-quinn

LGTM!

…NTMDB-802

themantissa

One tiny version edit and it's good to go! Other comment is about style in general, not blocking.

examples/federated-database-query-limit/README.md

themantissa · 2023-06-08T17:19:05Z

website/docs/d/federated_query_limits.html.markdown

@@ -18,7 +18,7 @@ description: |-
 ```terraform
 data "mongodbatlas_federated_query_limits" "test" {
  project_id = "PROJECT_ID"
-  tenant_name = "FEDERATED_DATABSE_INSTANCE_NAME"
+  tenant_name = "FEDERATED_DATABASE_INSTANCE_NAME"


Not blocking but nit - we have one style in one data source in another in this one. We really need to settle on this with a style guide. I personally prefer real values but I'm okay with this format if the team/docs think it's better. But either way we should find a way to be consistent. cc @Zuhairahmed

Sounds good and agree @themantissa. Found time in a few weeks with Docs Team and others to align on both a style guide and after that go through each resource by resource to make sure we are being consistent.

Co-authored-by: Andrea Angiolillo <[email protected]>

themantissa

Thank you for the change. LGTM!

* fix: microsoft_teams_webhook_url keeps updating on every apply (#1148) * INTMDB-783: Point in Time Restore is not enabled when should_copy_oplogs is set to true, when copying backups to other regions (#1150) * Rebase v1.10.0 (#1156) * INTMDB-710: Serverless Instance wants to do an in-place update on every run (#1152) * INTMDB-694: Add PrivateEndpoint.srvShardOptimizedConnectionString to cluster (#1157) * INTMDB-780: analyzer argument in Atlas search index is required (issue #1132) (#1158) * INTMDB-809: upgrade atlas go-client to v0.26.0 (#1164) * INTMDB-809: upgrade atlas go-client * Update go.mod * remain doc updates * INTMDB-801: [Terraform] Create new TF Federated Database Instance resources and data sources (#1163) * INTMDB-408: Remove Deprecated Resources - cloud_provider resources, private_ip_mode, NEW_RELIC and FLOWDOCK in third_party_integration resource (#1159) * Remove deprecated resources * Deprecate additional related resource parameters * Add log message * Remove mongodbatlas_private_ip_mode * Add Full deprecation notice to Docs * more doc updates --------- Co-authored-by: Zuhair Ahmed <[email protected]> * INTMDB-466 - Added is_extended_storage_sizes_enabled (#1128) * Added is_extended_storage_sizes enabled * Fixed lint complaints * INTMDB-834: Address Follow Up comments in INTMDB-804 (#1181) * node_count docs update * INTMDB-804: [Terraform] Create new TF Data Lake Pipelines resource and data sources. (#1174) * INTMDB-805: [Terraform] Create new TF Data Lake Pipelines Run data sources only (not resource) (#1177) * INTMDB-803: [Terraform] Create a new Private Endpoint resource and data sources which supports Federated Database Instance and Online Archive (#1182) * INTMDB-806: Deprecate "mongodbatlas_data_lake" and "privatelink_endpoint_service_adl" (#1190) * Make aws field optional in federated_database_instance resource and upgrade aws provider (#1205) * INTMDB-835: Create resource mongodbatlas_cluster_outage_simulation (#1188) * Fix typos in docs for network peering resource imports (#1200) * Remove delete from create update (#1209) * INTMDB-781: [Terraform] Parameter Add: retainBackups in Cluster and Advanced_Cluster (#1210) * INTMDB-856: Add cloudProviderConfig to mongodbatlas_federated_database_instance (#1215) * INTMDB-655: PAK Resource Updates + Doc Cleanup + Deprecation Warnings (#1208) * Add project_assignment feature to permit assigning multiple projects to an API key * Add deprecation * lint * Add examples for project and org API key * Add additional multi project example * Update versions.tf * Add deprecation message create function to remove duplicated code * lint * Add new deprecation message * Bump terraform version * Update project_api_key.html.markdown * Update project.html.markdown * Update docs * Add validation of project_assignment parameter * Create README.md * Update README.md * Create PAK-upgrade-guide-1.10.0.html.markdown * Update PAK-upgrade-guide-1.10.0.html.markdown * Update project_api_key.html.markdown * Update website/docs/r/access_list_api_key.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/d/access_list_api_key.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update examples/atlas-api-key/create-and-assign-pak-together/README.md Co-authored-by: zach-carr <[email protected]> * Update examples/atlas-api-key/create-api-key-assign-to-multiple-projects/README.md Co-authored-by: zach-carr <[email protected]> * Update examples/atlas-api-key/README.md Co-authored-by: zach-carr <[email protected]> * Update website/docs/r/project.html.markdown Co-authored-by: Zuhair Ahmed <[email protected]> * Update examples/atlas-api-key/create-and-assign-pak-seperately/README.md Co-authored-by: zach-carr <[email protected]> * Update examples/atlas-api-key/create-and-assign-pak-seperately/README.md Co-authored-by: Zuhair Ahmed <[email protected]> * Update examples/atlas-api-key/create-and-assign-pak-seperately/README.md Co-authored-by: zach-carr <[email protected]> * Add additional deprecation note * Update README.md * Bump up SDK to v0.29.0 * Update README.md More details to Readme explaining that all API keys are Organization Keys to help users --------- Co-authored-by: Zuhair Ahmed <[email protected]> Co-authored-by: Melissa Plunkett <[email protected]> Co-authored-by: zach-carr <[email protected]> * INTMDB-533: Feature Add: Programmatically Create Organizations (#1176) * Initial version * Acceptance testing issue for org creation * Add initial examples of org creation * skip tests for moment on organization resource * Add documentation * typo * terraform fmt * lint * go mod tidy * Documentation updates PR feedback * update TF version * Update variables.tf * Add additional Readme to show how to apply 2 step example * Update docs * Update examples/atlas-organization/Readme.md Co-authored-by: Zuhair Ahmed <[email protected]> * Update examples/atlas-organization/Readme.md * Update examples/atlas-organization/organization-step-1/Readme.md * Update examples/atlas-organization/organization-step-2/Readme.md Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/r/organization.html.markdown * Update website/docs/d/organizations.html.markdown * Update go SDK add Org Update * Fix broken tests * Add isDeleted * Update go.sum --------- Co-authored-by: Zuhair Ahmed <[email protected]> Co-authored-by: Melissa Plunkett <[email protected]> * INTMDB-802: Create new TF Data Federation Query Limit resource and data sources (#1173) * INTMDB-802: Add data_source and resource for Federated Database Query Limit * INTMDB-802: update provider.go * INTMDB-802: Add AtlasDataFederation Query Limit resource * INTMDB-802: Add AtlasDataFederation Query Limit resource * Update data_source_mongodbatlas_federated_query_limit.go * Update resource_mongodbatlas_federated_query_limit.go * INTMDB-802: temp * INTMDB-802: Add example * INTMDB-802: Add example * INTMDB-802: minor * INTMDB-802: example fix * Acceptance tests * Acceptance tests * Fix docs * Update main.tf * Update mongodbatlas/resource_mongodbatlas_federated_query_limit.go Co-authored-by: Andrea Angiolillo <[email protected]> * Update mongodbatlas/resource_mongodbatlas_federated_query_limit.go Co-authored-by: Andrea Angiolillo <[email protected]> * Update mongodbatlas/data_source_mongodbatlas_federated_query_limit.go Co-authored-by: Andrea Angiolillo <[email protected]> * Update mongodbatlas/resource_mongodbatlas_federated_query_limit.go Co-authored-by: Andrea Angiolillo <[email protected]> * Update mongodbatlas/resource_mongodbatlas_federated_query_limit.go Co-authored-by: Andrea Angiolillo <[email protected]> * Update website/docs/d/federated_query_limit.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/d/federated_query_limit.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/r/federated_query_limit.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/r/federated_query_limit.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/r/federated_query_limit.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/d/federated_query_limits.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update website/docs/r/federated_query_limit.html.markdown Co-authored-by: Melissa Plunkett <[email protected]> * Update federated_query_limit.html.markdown * Update federated_query_limit.html.markdown * Address PR comments * Address PR comments * Update website/docs/d/federated_query_limit.html.markdown Co-authored-by: Zuhair Ahmed <[email protected]> * Update website/docs/d/federated_query_limits.html.markdown Co-authored-by: Zuhair Ahmed <[email protected]> * Update examples/federated-database-query-limit/README.md Co-authored-by: Zuhair Ahmed <[email protected]> * Update examples/federated-database-query-limit/README.md Co-authored-by: Zuhair Ahmed <[email protected]> * Update examples/federated-database-query-limit/README.md Co-authored-by: Zuhair Ahmed <[email protected]> * Update federated_query_limit.html.markdown * Update federated_query_limits.html.markdown * Update README.md * Address PR comments * Address PR comments * Apply suggestions from code review Co-authored-by: zach-carr <[email protected]> * INTMDB-805: [Terraform] Create new TF Data Lake Pipelines Run data sources only (not resource) (#1177) * INTMDB-803: [Terraform] Create a new Private Endpoint resource and data sources which supports Federated Database Instance and Online Archive (#1182) * Fix AWS provider version and example * updated federated instance * Addressed Comments * Update mongodbatlas/data_source_mongodbatlas_federated_query_limits.go Co-authored-by: Andrea Angiolillo <[email protected]> * Update data_source_mongodbatlas_federated_query_limits.go --------- Co-authored-by: Andrea Angiolillo <[email protected]> Co-authored-by: Melissa Plunkett <[email protected]> Co-authored-by: Zuhair Ahmed <[email protected]> Co-authored-by: zach-carr <[email protected]> * Update .github_changelog_generator * Update CHANGELOG.md --------- Co-authored-by: maastha <[email protected]> Co-authored-by: Andrea Angiolillo <[email protected]> Co-authored-by: Zuhair Ahmed <[email protected]> Co-authored-by: Dosty Everts <[email protected]> Co-authored-by: Johanna Öjeling <[email protected]> Co-authored-by: Melissa Plunkett <[email protected]> Co-authored-by: zach-carr <[email protected]>

maastha and others added 13 commits May 15, 2023 17:17

INTMDB-802: Add data_source and resource for Federated Database Query…

0edb8e3

… Limit

INTMDB-802: update provider.go

085be14

INTMDB-802: Add AtlasDataFederation Query Limit resource

5accd9d

INTMDB-802: Add AtlasDataFederation Query Limit resource

8deb8a5

Update data_source_mongodbatlas_federated_query_limit.go

5529a94

Update resource_mongodbatlas_federated_query_limit.go

3fe2ae0

INTMDB-802: temp

b28e6f5

Merge branch 'release-staging-v.1.10.0' into INTMDB-802

da92b05

INTMDB-802: Add example

5704fbb

INTMDB-802: Add example

13e4998

INTMDB-802: minor

8a026a3

INTMDB-802: example fix

c04684a

Acceptance tests

8dcee52

maastha changed the base branch from master to release-staging-v.1.10.0 May 25, 2023 07:34

maastha added 2 commits May 25, 2023 08:51

Merge branch 'release-staging-v.1.10.0' into INTMDB-802

d8eefab

Acceptance tests

76143b3

maastha changed the title ~~Intmdb 802~~ INTMDB-802: Create new TF Data Federation Query Limit resource and data sources May 25, 2023

maastha marked this pull request as ready for review May 25, 2023 08:40

maastha requested a review from a team as a code owner May 25, 2023 08:40

maastha requested review from martinstibbe, Zuhairahmed, themantissa and zach-carr May 25, 2023 08:43

maastha and others added 2 commits May 25, 2023 09:54

Fix docs

17a61c2

Update main.tf

1cd8d7c

andreaangiolillo reviewed May 30, 2023

View reviewed changes

andreaangiolillo added the enhancement label May 30, 2023

maastha and others added 2 commits May 30, 2023 13:03

Update mongodbatlas/resource_mongodbatlas_federated_query_limit.go

a0b1714

Co-authored-by: Andrea Angiolillo <[email protected]>

Update mongodbatlas/resource_mongodbatlas_federated_query_limit.go

21dfe22

Co-authored-by: Andrea Angiolillo <[email protected]>

themantissa requested changes May 31, 2023

View reviewed changes

martinstibbe approved these changes Jun 1, 2023

View reviewed changes

andreaangiolillo requested review from Zuhairahmed and themantissa June 2, 2023 10:09

zach-carr approved these changes Jun 2, 2023

View reviewed changes

examples/federated-database-query-limit/README.md Outdated Show resolved Hide resolved

examples/federated-database-query-limit/variables.tf Outdated Show resolved Hide resolved

maastha and others added 5 commits June 2, 2023 17:24

Apply suggestions from code review

6c690dc

Co-authored-by: zach-carr <[email protected]>

INTMDB-805: [Terraform] Create new TF Data Lake Pipelines Run data so…

e68dff1

…urces only (not resource) (#1177)

INTMDB-803: [Terraform] Create a new Private Endpoint resource and da…

473d316

…ta sources which supports Federated Database Instance and Online Archive (#1182)

Merge branch 'release-staging-v.1.10.0' into INTMDB-802

fae6f14

Fix AWS provider version and example

4ebb85e

Zuhairahmed approved these changes Jun 2, 2023

View reviewed changes

andreaangiolillo added 2 commits June 5, 2023 10:08

fix merge

7030c34

Merge branch 'INTMDB-802' of github.com:mongodb/terraform-provider-mo…

f8d01b1

…ngodbatlas into INTMDB-802

andreaangiolillo approved these changes Jun 5, 2023

View reviewed changes

colm-quinn approved these changes Jun 7, 2023

View reviewed changes

andreaangiolillo mentioned this pull request Jun 7, 2023

INTMDB-856: Add cloudProviderConfig to mongodbatlas_federated_database_instance #1215

Merged

10 tasks

andreaangiolillo added 2 commits June 7, 2023 21:22

Merge remote-tracking branch 'origin/release-staging-v.1.10.0' into I…

665f9a1

…NTMDB-802

updated federated instance

e21aecc

themantissa requested changes Jun 8, 2023

View reviewed changes

Addressed Comments

daf8930

andreaangiolillo requested a review from themantissa June 8, 2023 19:47

Zuhairahmed and others added 2 commits June 8, 2023 23:31

Update mongodbatlas/data_source_mongodbatlas_federated_query_limits.go

0b8794f

Co-authored-by: Andrea Angiolillo <[email protected]>

Update data_source_mongodbatlas_federated_query_limits.go

172faa5

themantissa approved these changes Jun 9, 2023

View reviewed changes

martinstibbe merged commit 9443fb2 into release-staging-v.1.10.0 Jun 10, 2023

martinstibbe deleted the INTMDB-802 branch June 10, 2023 02:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTMDB-802: Create new TF Data Federation Query Limit resource and data sources #1173

INTMDB-802: Create new TF Data Federation Query Limit resource and data sources #1173

maastha commented May 17, 2023 •

edited

Loading

maastha commented May 25, 2023 •

edited

Loading

andreaangiolillo May 30, 2023

maastha May 30, 2023

themantissa May 30, 2023

maastha May 31, 2023

Zuhairahmed May 31, 2023

Zuhairahmed Jun 2, 2023 •

edited

Loading

themantissa Jun 6, 2023

Zuhairahmed Jun 7, 2023

andreaangiolillo Jun 7, 2023

andreaangiolillo Jun 7, 2023

themantissa left a comment

themantissa May 31, 2023

martinstibbe left a comment

zach-carr left a comment

maastha commented Jun 2, 2023

Zuhairahmed left a comment

andreaangiolillo commented Jun 5, 2023

Zuhairahmed commented Jun 5, 2023

colm-quinn left a comment

themantissa left a comment

themantissa Jun 8, 2023

Zuhairahmed Jun 9, 2023

themantissa left a comment

INTMDB-802: Create new TF Data Federation Query Limit resource and data sources #1173

INTMDB-802: Create new TF Data Federation Query Limit resource and data sources #1173

Conversation

maastha commented May 17, 2023 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

maastha commented May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zuhairahmed Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themantissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinstibbe left a comment

Choose a reason for hiding this comment

zach-carr left a comment

Choose a reason for hiding this comment

maastha commented Jun 2, 2023

Zuhairahmed left a comment

Choose a reason for hiding this comment

andreaangiolillo commented Jun 5, 2023

Zuhairahmed commented Jun 5, 2023

colm-quinn left a comment

Choose a reason for hiding this comment

themantissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themantissa left a comment

Choose a reason for hiding this comment

maastha commented May 17, 2023 •

edited

Loading

maastha commented May 25, 2023 •

edited

Loading

Zuhairahmed Jun 2, 2023 •

edited

Loading