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: support new tags attribute in cluster, advanced_cluster, and serverless_instance #1461

Merged
merged 6 commits into from
Sep 18, 2023

Conversation

AgustinBettati
Copy link
Member

@AgustinBettati AgustinBettati commented Sep 8, 2023

Description

Ticket: INTMDB-515

  • Support new tags attribute in cluster, advanced_cluster, and serverless_instance resource and data sources.
  • Includes deprecation for labels attribute defined in cluster, advanced_cluster.

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

Testing comments

Acceptance tests

  • under the resource test file of cluster, advanced_cluster, and serverless_instance a new test case was added to verify creation and update of tags. These test cases include verification of the resource and data sources in the same test for time reasons (both running locally and in CI).

Manual verification

  • local binary was built and used to create a cluster, advanced_cluster, and serverless instance. Additional the respective data sources were used to get the created clusters and verify tags are present.

Comment on lines +345 to +347
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Resource{
Copy link
Member Author

@AgustinBettati AgustinBettati Sep 8, 2023

Choose a reason for hiding this comment

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

When comparing to labels attribute schema, no Set function is defined as it is not needed. Using hashicorp/terraform#10520 (comment) as reference, the default implementation will properly hash the entire data structure.

@AgustinBettati AgustinBettati marked this pull request as ready for review September 12, 2023 08:13
@AgustinBettati AgustinBettati requested a review from a team as a code owner September 12, 2023 08:13
Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Infrastructure as Code high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Vulnerabilities high 0   medium 0   low 0   info 0 View in Orca
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

Computed: true,
Type: schema.TypeSet,
Computed: true,
Deprecated: fmt.Sprintf(DeprecationByDateWithReplacement, "November 2023", "tags"),
Copy link
Member

Choose a reason for hiding this comment

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

it might be useful to have constants with deprecation dates so it's easier to see next deprecation dates

Copy link
Member Author

Choose a reason for hiding this comment

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

This could be helpful, but I believe configuring due date within jira tickets will be our best bet to make sure we are aware of removing deprecated fields.

resource.TestCheckResourceAttr(dataSourceName, "tags.#", "2"),
resource.TestCheckResourceAttr(dataSourceName, "tags.0.key", "key 1"),
resource.TestCheckResourceAttr(dataSourceName, "tags.0.value", "value 1"),
resource.TestCheckResourceAttr(dataSourceName, "tags.1.key", "key 2"),
Copy link
Member

Choose a reason for hiding this comment

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

it'd be nice to have a less verbose way to check tags, or other attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree.
I have looked into one alternative to simplify defining assertions of the terraform state, this is the PoC: #1448.

@@ -74,15 +75,25 @@ Specifies BI Connector for Atlas configuration.
* `enabled` - Specifies whether or not BI Connector for Atlas is enabled on the cluster.l
* `read_preference` - Specifies the read preference to be used by BI Connector for Atlas on the cluster. Each BI Connector for Atlas read preference contains a distinct combination of [readPreference](https://docs.mongodb.com/manual/core/read-preference/) and [readPreferenceTags](https://docs.mongodb.com/manual/core/read-preference/#tag-sets) options. For details on BI Connector for Atlas read preferences, refer to the [BI Connector Read Preferences Table](https://docs.atlas.mongodb.com/tutorial/create-global-writes-cluster/#bic-read-preferences).

### Tags

Key-value pairs used for tagging and categorizing the cluster. Each key and value has a maximum length of 255 characters.
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any check here for limit 255 in key & values, shouldn't we do it here or it's ok if it's only done in the SDK?

Copy link
Member Author

Choose a reason for hiding this comment

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

Within the terraform provider we currently are not defining any validations, such as regex, min/max lengths, or enums. We could analyse what benefit benefit it brings to the user taking into account the additional maintenance we add to the provider.

Copy link

@corryroot corryroot left a comment

Choose a reason for hiding this comment

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

Thanks @AgustinBettati! I commented with mostly copy nits. I do recommend that you use a dochub URL instead of a direct link to the page. https://dochub.mongodb.org/core/add-cluster-tag-atlas.

website/docs/d/advanced_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/d/advanced_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/d/advanced_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/d/advanced_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/d/advanced_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/serverless_instance.html.markdown Outdated Show resolved Hide resolved
website/docs/r/serverless_instance.html.markdown Outdated Show resolved Hide resolved
website/docs/r/serverless_instance.html.markdown Outdated Show resolved Hide resolved
Copy link

@corryroot corryroot left a comment

Choose a reason for hiding this comment

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

LGTM!

@AgustinBettati
Copy link
Member Author

will merge, if any future comments appear will address in a followup PR.

@AgustinBettati AgustinBettati merged commit 0a727bb into master Sep 18, 2023
21 checks passed
@AgustinBettati AgustinBettati deleted the INTMDB-515 branch September 18, 2023 07:11
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.

4 participants