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

doc: Adjust documentation of search_deployment resource and data source #1833

Merged
merged 6 commits into from Jan 15, 2024

Conversation

andreaangiolillo
Copy link
Collaborator

@andreaangiolillo andreaangiolillo commented Jan 12, 2024

Description

Ticket: https://jira.mongodb.org/browse/CLOUDP-221359

This PR updates the search_deployment doc via make generate-doc

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

@andreaangiolillo andreaangiolillo marked this pull request as ready for review January 12, 2024 15:37
@andreaangiolillo andreaangiolillo requested a review from a team as a code owner January 12, 2024 15:37
@@ -9,6 +9,7 @@ import (

func DataSourceSchema(ctx context.Context) schema.Schema {
return schema.Schema{
MarkdownDescription: "Provides a Search Deployment data source.",
Copy link
Member

Choose a reason for hiding this comment

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

I thought at the end we would generate both MarkdowDescription and Description cc @AgustinBettati

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I added this manually, we could also move this to the template instead. Wondering if there is a way to get the resource description from the open api spec instead. CC @AgustinBettati in case you know how to do this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looking at the Hashicorp doc https://developer.hashicorp.com/terraform/plugin/code-generation/openapi-generator#generator-config it does not seem possible. I will open an issue on their repo. In the meantime I will remove this field from the schema and move the description to the template

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I agree moving to template will be the best option for now

@@ -1,37 +1,77 @@
---
layout: "mongodbatlas"
page_title: "MongoDB Atlas: search deployment"
sidebar_current: "docs-mongodbatlas-datasource-search-deployment"
page_title: "MongoDB Atlas: mongodbatlas_search_deployment"
Copy link
Member

Choose a reason for hiding this comment

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

were these deleted/added headers wrong? should we change them in all resources / ds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I double-checked this with Zuhair and he confirmed that this metadata is no longer used by Hashicorp. Hashicorp used this metadata for screen readers which does not seem to be the case anymore.


* `instance_size` - (Required) Hardware specification for the search node instance sizes. The [MongoDB Atlas API](https://www.mongodb.com/docs/atlas/reference/api-resources-spec/#tag/Atlas-Search/operation/createAtlasSearchDeployment) describes the valid values. More details can also be found in the [Search Node Documentation](https://www.mongodb.com/docs/atlas/cluster-config/multi-cloud-distribution/#search-tier).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting: are we losing some valuable information by removing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the description is not good enough, we should update the open API spec of the endpoint. In this case, I will open a follow-up PR to add "More details can also be found in the Search Node Documentation" to the endpoint. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI created this ticket https://jira.mongodb.org/browse/CLOUDP-222701 to update the open api spec


### Optional

- `timeouts` (Attributes) (see [below for nested schema](#nestedatt--timeouts))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can you just check that we are not losing some relevant information here with the change of timeouts field compared to how it was before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO we are getting more information for timeouts
Screenshot 2024-01-15 at 09 27 53.

I see that this does not apply to instance_size. If we think that the description is not good enough, we should update the open API spec of the endpoint

@andreaangiolillo andreaangiolillo merged commit 8a5ba5f into master Jan 15, 2024
35 checks passed
@andreaangiolillo andreaangiolillo deleted the CLOUDP-221359_2 branch January 15, 2024 09:35
Copy link
Collaborator

@AgustinBettati AgustinBettati left a comment

Choose a reason for hiding this comment

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

Some comments after comparing generated documentation using https://registry.terraform.io/tools/doc-preview


### Specs
* `instance_size` - (Required) Hardware specification for the search node instance sizes. The [MongoDB Atlas API](https://www.mongodb.com/docs/atlas/reference/api-resources-spec/#tag/Atlas-Search/operation/createAtlasSearchDeployment) describes the valid values. More details can also be found in the [Search Node Documentation](https://www.mongodb.com/docs/atlas/cluster-config/multi-cloud-distribution/#search-tier).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would preserve this more detailed description of the instance size attribute as it also references valid values, we can adjust the schema description fields and also include a description override config in the generator_config.yml file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was planning to modify this in the open api spec but this may be better. I will also update our doc to make sure this is documented

Comment on lines -11 to -15
`mongodbatlas_search_deployment` provides a Search Deployment resource. The resource lets you create, edit and delete dedicated search nodes in a cluster.

-> **NOTE:** For details on supported cloud providers and existing limitations you can visit the [Search Node Documentation](https://www.mongodb.com/docs/atlas/cluster-config/multi-cloud-distribution/#search-nodes-for-workload-isolation).

-> **NOTE:** Only a single search deployment resource can be defined for each cluster.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer to not remove this content as it is valuable for the docs. Could we add this in the template, or as part of the description field of the schema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you clarify the content that you want to include in the template? is the NOTE?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes the 2 NOTE: provide valuable info/clarification.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, we should add them in the template in this case


* `project_id` - (Required) Unique 24-hexadecimal digit string that identifies your project.
* `cluster_name` - (Required) Label that identifies the cluster to create search nodes for.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should preserve this description as it makes more sense within the context of a resource.

@andreaangiolillo
Copy link
Collaborator Author

@AgustinBettati I will open a follow-up PR to address these comments.

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

Successfully merging this pull request may close these issues.

None yet

4 participants