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.

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

feat: mongodbatlas_search_deployment data source with acceptance tests and docs #1621

Closed
wants to merge 1 commit into from

Conversation

AgustinBettati
Copy link
Member

@AgustinBettati AgustinBettati commented Nov 14, 2023

Description

Jira ticket: INTMDB-1273

Currently open as draft pull request as we have to make use of a dev preview of the Atlas SDK.

This PR includes:

  • Implementation of mongodbatlas_search_deployment data source
  • acceptance tests
  • 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 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 changed the title feat: search node data source with acceptance tests and docs feat: search_deployment data source with acceptance tests and docs Nov 15, 2023
@AgustinBettati AgustinBettati changed the title feat: search_deployment data source with acceptance tests and docs feat: mongodbatlas_search_deployment data source with acceptance tests and docs Nov 15, 2023
Copy link
Contributor

Code Coverage

Package Line Rate Health
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas 2%
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas/framework/validator 68%
github.com/mongodb/terraform-provider-mongodbatlas/mongodbatlas/util 17%
Summary 3% (272 / 10493)

DSCommon
}

func (d *SearchDeploymentDS) Schema(ctx context.Context, req datasource.SchemaRequest, resp *datasource.SchemaResponse) {
Copy link
Collaborator

@marcosuma marcosuma Nov 15, 2023

Choose a reason for hiding this comment

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

this is more of a generic question: in the context of a data source I don't understand the concept of Computed: aren't they all computed?

On the other side I'd imagine Required means that if we don't fill it up with the backend response terraform would raise an error, but why do we really need to set it as required? Couldn't we just leave it as Optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

in the context of a data source, aren't they all computed?

Not exactly. When using a data source there are some attributes that need to be provided by the user in order to identify the resource that will be fetched. In this case the resource is identified through the project_id and cluster_name, which is why they are marked as required.
You will see this is also clarified in the documentation of the data source, separating the concept of arguments (attributes that have to be defined), and then an attributes sections for values that are exported (or as we call computed).

@@ -31,12 +31,13 @@ var _ resource.ResourceWithImportState = &SearchDeploymentRS{}

const (
searchDeploymentDoesNotExistsError = "ATLAS_FTS_DEPLOYMENT_DOES_NOT_EXIST"
searchDeploymentName = "search_deployment"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this looks like a separate PR? no need to do it now, but consider it for the next time

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes true. Tried to keep this PR as clean as possible with respect to the resource implementation, but you usually some pending details always get adjusted when implementing the data source.

@AgustinBettati
Copy link
Member Author

merged in #1633

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants