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.

Sign up for GitHub

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

Jump to bottom

feat: new mongodbatlas_stream_connection and mongodbatlas_stream_connections data sources #1757

Merged
merged 3 commits into from
Dec 15, 2023
Merged

feat: new mongodbatlas_stream_connection and mongodbatlas_stream_connections data sources #1757

merged 3 commits into from
Dec 15, 2023

Conversation

AgustinBettati
Copy link
Member

Description

Link to any related issue(s): CLOUDP-215323

Includes unit testing, acceptance tests, and documentation.

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 checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
  • 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
  • If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

@AgustinBettati AgustinBettati mentioned this pull request Dec 15, 2023
Merged
12 tasks
Base automatically changed from CLOUDP-215322-avoid-depending-on-existing-proj to CLOUDP-215322 December 15, 2023 09:20
@AgustinBettati AgustinBettati force-pushed the CLOUDP-215323 branch 2 times, most recently from 366b531 to 7ce5f8d Compare December 15, 2023 10:30
AgustinBettati
AgustinBettati commented Dec 15, 2023
View reviewed changes
website/docs/d/stream_connection.html.markdown Outdated


To learn more, see: [MongoDB Atlas API - Stream Connection](https://www.mongodb.com/docs/atlas/reference/api-resources-spec/#tag/Streams/operation/getStreamConnection) Documentation.
The [Terraform Provider Examples Section](https://github.com/mongodb/terraform-provider-mongodbatlas/blob/master/examples/atlas-streams/README.md) also contains details on the overall support for Atlas Streams in Terraform.
Copy link
Member Author

Choose a reason for hiding this comment

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

referenced README file is defined in #1752

@AgustinBettati AgustinBettati marked this pull request as ready for review December 15, 2023 10:51
@AgustinBettati AgustinBettati requested a review from a team as a code owner December 15, 2023 10:51
andreaangiolillo
andreaangiolillo approved these changes Dec 15, 2023
View reviewed changes
Copy link
Collaborator

@andreaangiolillo andreaangiolillo left a comment

Choose a reason for hiding this comment

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

LGTM

internal/service/streamconnection/data_source_stream_connection_test.go

func TestAccStreamDSStreamConnection_kafkaPlaintext(t *testing.T) {
var (
orgID = os.Getenv("MONGODB_ATLAS_ORG_ID")
Copy link
Collaborator

Choose a reason for hiding this comment

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

[nit] not need to address it here. I am wondering if we should define MONGODB_ATLAS_ORG_ID and other env vars in a common file and then import them. In this way, if we change the env var we just need to update it in one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

could remove some boilerplate for sure, on the other hand it does help to see want env variables are being used for each test so dont have a strong opinion

internal/service/streamconnection/data_source_stream_connections_test.go
CheckDestroy: CheckDestroyStreamConnection,
Steps: []resource.TestStep{
{
Config: streamConnectionsDataSourceConfig(kafkaStreamConnectionConfig(orgID, projectName, instanceName, "user", "rawpassword", "localhost:9092,localhost:9092", "earliest", false)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

[q - not blocking] Since all the data source tests use a resource. Couldn't we just have one acceptance test file where we have data source and resource test together? I see how this would decrease the amount of resources that we need to run the acceptance tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a good point, I did go for this option in the case of search_deployment resource as execution times were quite long.
For this case the creation/update/delete of instance connections is very fast, so saw no harm in leaving in separate tests. One test case, Cluster connection type, does require creating a cluster but when running locally you can reference existing an existing cluster using env variables.

website/docs/d/stream_connection.html.markdown
@@ -0,0 +1,57 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to add a new example in the example folder?

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

Base automatically changed from CLOUDP-215322 to master December 15, 2023 15:29
erabil-mdb
erabil-mdb approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@erabil-mdb erabil-mdb left a comment

Choose a reason for hiding this comment

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

LGTM, but added some style nits!

@github-actions GitHub Actions
Copy link
Contributor

Code Coverage

Package Line Rate Health
github.com/mongodb/terraform-provider-mongodbatlas/internal/common/conversion 31% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/common/dsschema 100% ✔
github.com/mongodb/terraform-provider-mongodbatlas/internal/common/validate 68% ➖
github.com/mongodb/terraform-provider-mongodbatlas/internal/provider 7% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/accesslistapikey 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/advancedcluster 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/alertconfiguration 33% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/apikey 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/atlasuser 0% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/auditing 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/backupcompliancepolicy 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cloudbackupschedule 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cloudbackupsnapshot 5% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cloudbackupsnapshotexportbucket 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cloudbackupsnapshotexportjob 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cloudbackupsnapshotrestorejob 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cloudprovideraccess 4% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/cluster 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/clusteroutagesimulation 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/customdbrole 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/customdnsconfigurationclusteraws 5% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/databaseuser 30% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/datalakepipeline 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/encryptionatrest 31% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/eventtrigger 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federateddatabaseinstance 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedquerylimit 4% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedsettingsidentityprovider 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedsettingsorgconfig 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/federatedsettingsorgrolemapping 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/globalclusterconfig 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/ldapconfiguration 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/ldapverify 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/maintenancewindow 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/networkcontainer 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/networkpeering 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/onlinearchive 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/organization 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/orginvitation 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/privateendpointregionalmode 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/privatelinkendpoint 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/privatelinkendpointserverless 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/privatelinkendpointservice 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/privatelinkendpointservicedatafederationonlinearchive 3% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/privatelinkendpointserviceserverless 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/project 30% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/projectapikey 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/projectinvitation 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/projectipaccesslist 10% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/rolesorgid 7% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/searchdeployment 23% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/searchindex 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/serverlessinstance 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/streamconnection 26% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/streaminstance 14% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/teams 1% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/thirdpartyintegration 4% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/service/x509authenticationdatabaseuser 2% ❌
github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/acc 4% ❌
Summary 7% (765 / 11026) ❌

@AgustinBettati AgustinBettati merged commit 77d4140 into master Dec 15, 2023
@AgustinBettati AgustinBettati deleted the CLOUDP-215323 branch December 15, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lantoli lantoli lantoli approved these changes

@andreaangiolillo andreaangiolillo andreaangiolillo approved these changes

@erabil-mdb erabil-mdb erabil-mdb approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AgustinBettati @lantoli @andreaangiolillo @erabil-mdb