chore: Creates `advancedclustertpf` package for new implementation #2679

lantoli · 2024-10-11T12:07:33Z

Description

Creates advancedclusterold package for current implementation and new no-op resource.

In this way both current and TPF-to-be-built mongodbatlas_advanced_cluster versions can co-exist together.

advancedcluster package has the current implementation for mongodbatlas_advanced_cluster
advancedclustertpf has been created as if it was a new resource, using resource and schema scaffolding
advancedcluster in this PR is a no-op resource so it can be created and deleted, but doesn't call Atlas
make goal has been done to activate the new resource in CI tests
some adjustments has been done to scaffolding template files

Important commits:

Commit to make it compile after resource & schema generation, good reference to see what auto-generated code has been removed or changed, and what schema adjustments (mostly deleted duplicated structs) have been done, can be useful in following PRs: 5307394
Adjustments in scaffolding templates and how generated code changes: b9c320a

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

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. A migration guide must be created or updated if the new feature will go in a major version.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contributing guides
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 have added appropriate changelog entries.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

make scaffold resource_name=advanced_cluster type=resource make scaffold resource_name=advanced_cluster type=data-resource make scaffold resource_name=advanced_cluster type=plural-data-source

make scaffold-schemas resource_name=advancedCluster

lantoli · 2024-10-14T05:51:59Z

.github/workflows/acceptance-tests-runner.yml

@@ -331,12 +334,36 @@ jobs:
        with:
          terraform_version: ${{ inputs.terraform_version }}
          terraform_wrapper: false  
+      - name: Prepare new advanced_cluster
+        run: make tools enable-new-advancedcluster


activate new resource implementation before running its CI tests

lantoli · 2024-10-14T05:52:44Z

.golangci.yml

@@ -123,6 +123,8 @@ issues:
    - linters:
        - gocritic
      text: "^hugeParam: req is heavy"
+    - path: "_schema\\.go" # exclude rules for schema files as it's auto-genereated from OpenAPI spec
+      text: "fieldalignment|hugeParam|var-naming|ST1003|S1007|exceeds the maximum|too long|regexpSimplify|nolint"


linter exceptions for auto-generate schemas so we don't need to manually change them

lantoli · 2024-10-14T05:53:02Z

GNUmakefile

@@ -153,3 +154,24 @@ check-changelog-entry-file:
 .PHONY: jira-release-version
 jira-release-version:
 	go run ./tools/jira-release-version/*.go
+
+.PHONY: enable-new-advancedcluster


make target to enable new resource, can also be run in local

lantoli · 2024-10-14T05:53:48Z

internal/service/advancedcluster/README.md

@@ -0,0 +1,9 @@
+# advancedcluster package


README files in adv_clusters to help with the transition

lantoli · 2024-10-14T06:06:31Z

internal/service/advancedcluster/resource_test.go

+
+func configBasic() string {
+	return `	
+		resource "mongodbatlas_advanced_cluster" "test" {


minimum test for the no-op resource, it creates and deletes the resource

lantoli · 2024-10-14T06:11:39Z

tools/scaffold/template/datasource.tmpl

@@ -9,32 +9,32 @@ import (
 	"github.com/mongodb/terraform-provider-mongodbatlas/internal/config"
 )

-const {{.NameCamelCase}}Name = "{{.NameSnakeCase}}" // TODO: if resource exists this can be deleted


scaffolding adjustments, all of them can be seen in this PR and how a resource changes: b9c320a

I can also move the scaffolding changes to a different PRs if that's preferred.

maastha · 2024-10-14T06:56:30Z

tools/scaffold/scaffold.go

@@ -79,6 +79,10 @@ func filesToGenerate(params *ScaffoldParams) ([]FileGeneration, error) {
 				TemplatePath: "tools/scaffold/template/generator_config.tmpl",
 				OutputPath:   fmt.Sprintf("%s/tfplugingen/generator_config.yml", folderPath),
 			},
+			{


lantoli · 2024-10-14T07:08:42Z

tools/scaffold/scaffold.go

@@ -88,7 +92,7 @@ func filesToGenerate(params *ScaffoldParams) ([]FileGeneration, error) {
 			},
 			{
 				TemplatePath: "tools/scaffold/template/acc_test.tmpl",
-				OutputPath:   fmt.Sprintf("%s/data_source_test.go", folderPath),
+				OutputPath:   fmt.Sprintf("%s/resource_test.go", folderPath),


we only create test file for resource to have also data source tests

tools/scaffold/scaffold.go

tools/scaffold/template/model.tmpl

maastha · 2024-10-14T07:05:45Z

internal/service/advancedcluster/data_source.go

+	"github.com/mongodb/terraform-provider-mongodbatlas/internal/config"
+)
+
+const resourceName = "advanced_cluster" // TODO: if resource exists this can be deleted


remove comment?

i think it's ok to leave it as it helps to know that it has to be removed if resource exists (we get a build error when both constants are defined)

we get a build error when both constants are defined

exactly, that's why personally I don't see value in keeping it. Also, for this one resource does exist already so we have that sorted already no?
As in I don't see anything we need to TODO here anymore

sorry I misunderstood, I thought you meant to delete the comment not the full line. It's not usual but happens that we can have a data source but not a resource, in that case this is needed, e.g. control_plane_ip_addresses

I thought you meant to delete the comment

I was referring to delete the comment, yes. My point is since the comment is meant to serve as a guidance during scaffolding. Now that we have the resource and data sources figured out, the comment isn't needed anymore because there's nothing left TODO

for me it's a guide for the tool user, we don't do complex logic yet, so if you have a resource and data source you can see the comment and understand why it's there and delete it, right now we don't have logic to keep the line only if datasource exists

internal/service/advancedcluster/data_source_schema.go

maastha · 2024-10-14T07:17:43Z

.github/workflows/acceptance-tests-runner.yml

@@ -331,12 +334,36 @@ jobs:
        with:
          terraform_version: ${{ inputs.terraform_version }}
          terraform_wrapper: false  
+      - name: Prepare new advanced_cluster


The nightly tests now will invoke both these advancedcluster groups right?
If so, for a while we might have both tests creating cluster, could that make the Out of capacity issues worse or do you think we have that handled now?

i think capacity issues are fixed now and this shouldn't be a problem

understand are not a problem now, but aren't we doubling the amount of clusters? If so, I'd like to consider alternative solutions.

We can exclude tests for advancedclulster TPF group from nightly runs and run only in PRs based on change detection in that package

yeah, that's one option, only run them in PRs

AgustinBettati

LGTM

AgustinBettati · 2024-10-14T07:58:20Z

tools/scaffold/template/datasource.tmpl

-var _ datasource.DataSource = &{{.NameCamelCase}}DS{}
-var _ datasource.DataSourceWithConfigure = &{{.NameCamelCase}}DS{}
+var _ datasource.DataSource = &ds{}
+var _ datasource.DataSourceWithConfigure = &ds{}


nice additional changes to the scaffolding 👍

AgustinBettati · 2024-10-14T08:03:03Z

internal/service/advancedclusterold/data_source_advanced_clusters.go

-package advancedcluster
+package advancedclusterold


Since we will have a few months in this state, might prefer leaving existing advanced cluster implementation with advancedcluster package name and have new implementation with advancedclustertpf so there is no confusion.

thanks, done in this commit 368c56d

GNUmakefile

EspenAlbert · 2024-10-14T11:09:58Z

.golangci.yml

@@ -123,6 +123,8 @@ issues:
    - linters:
        - gocritic
      text: "^hugeParam: req is heavy"
+    - path: "_schema\\.go" # exclude rules for schema files as it's auto-genereated from OpenAPI spec


IMO: I don't like this change.
At least for now, I prefer doing these manual updates to the schema file and ensure linting passes. Also, I think all of the custom types should be removed.
Later when we introduce re-regenerating in the CI we can change this to be more permissive

there are too many changes to do, I've created a ticket to follow it, I hope that's fine: CLOUDP-278408

EspenAlbert

Great work and improvements!

* master: (44 commits) Remove RemoveResource from Create and Update (#2694) sleep first time only (#2696) fix: Ensures event_trigger `disabled` is included in the PUT payload (#2690) chore: Bump srvaroa/labeler from 1.11.0 to 1.11.1 (#2695) build(deps): bump go.mongodb.org/atlas-sdk (#2686) fix: Avoids setting organization id to empty when the USER_IS_NOT_FOUND (#2684) doc: Corrects default value of `auto_scaling_disk_gb_enabled` (#2683) chore: Reminder to log test failures (#2688) chore: Updates CHANGELOG.md for #2670 feat: Supports `config_server_type` and `config_server_management_mode` in advanced_cluster (#2670) chore: Creates `advancedclustertpf` package for new implementation (#2679) chore: Removes deprecation warnings for `labels` in cluster/adv_cluster (#2678) doc: Adjusts `redact_client_log_data` doc (#2676) chore: Adds sleep in tests to avoid out of capacity errors (#2674) refactor: Adjust type of set structure in team update logic (#2675) chore: Updates CHANGELOG.md header for v1.21.1 release chore: Updates examples link in index.md for v1.21.1 release build(deps): bump go.mongodb.org/atlas-sdk (#2672) doc: Fixes Public -> Private Preview and adds contact link. (#2671) chore: Updates CHANGELOG.md for #2669 ...

lantoli added 13 commits October 11, 2024 14:04

rename adv_cluster to old

37ddcc7

run:

ce633bc

make scaffold resource_name=advanced_cluster type=resource make scaffold resource_name=advanced_cluster type=data-resource make scaffold resource_name=advanced_cluster type=plural-data-source

update Atlas endpoints

60924bc

run:

de5a5cb

make scaffold-schemas resource_name=advancedCluster

README files

8a90674

change scaffolding models

b9c320a

exclude linter rules for schema auto-generated files

682824e

make it compile

5307394

CI config

a646b10

basic acc test

2e5dccd

scripts to enable new resource

ea1132a

no-op Create and Schema

68ce407

prepare new resource in CI

feb49dd

lantoli commented Oct 14, 2024

View reviewed changes

lantoli marked this pull request as ready for review October 14, 2024 06:11

lantoli requested a review from a team as a code owner October 14, 2024 06:11

maastha reviewed Oct 14, 2024

View reviewed changes

lantoli commented Oct 14, 2024

View reviewed changes

maastha reviewed Oct 14, 2024

View reviewed changes

AgustinBettati approved these changes Oct 14, 2024

View reviewed changes

lantoli added 2 commits October 14, 2024 12:01

rename to tpf

368c56d

typo

2ae52e1

lantoli changed the title ~~chore: Creates advancedclusterold package for current implementation and new no-op resource~~ chore: Creates advancedclustertpf package for new implementation Oct 14, 2024

EspenAlbert reviewed Oct 14, 2024

View reviewed changes

GNUmakefile Show resolved Hide resolved

EspenAlbert reviewed Oct 14, 2024

View reviewed changes

EspenAlbert approved these changes Oct 14, 2024

View reviewed changes

maastha approved these changes Oct 14, 2024

View reviewed changes

lantoli merged commit ab2f4f6 into master Oct 14, 2024
46 checks passed

lantoli deleted the CLOUDP-278191_old_adv_cluster branch October 14, 2024 13:03

lantoli mentioned this pull request Oct 16, 2024

chore: Generate Flex Cluster file structure and resource schema #2702

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Creates `advancedclustertpf` package for new implementation #2679

chore: Creates `advancedclustertpf` package for new implementation #2679

lantoli commented Oct 11, 2024 •

edited

Loading

lantoli Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024

lantoli Oct 14, 2024

lantoli Oct 14, 2024

lantoli Oct 14, 2024

lantoli Oct 14, 2024

maastha Oct 14, 2024

lantoli Oct 14, 2024

maastha Oct 14, 2024

lantoli Oct 14, 2024 •

edited

Loading

maastha Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024

maastha Oct 14, 2024

lantoli Oct 14, 2024 •

edited

Loading

maastha Oct 14, 2024

lantoli Oct 14, 2024 •

edited

Loading

marcosuma Oct 14, 2024

maastha Oct 14, 2024

lantoli Oct 14, 2024

AgustinBettati left a comment

AgustinBettati Oct 14, 2024

AgustinBettati Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024

EspenAlbert Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024 •

edited

Loading

EspenAlbert left a comment

chore: Creates advancedclustertpf package for new implementation #2679

chore: Creates advancedclustertpf package for new implementation #2679

Conversation

lantoli commented Oct 11, 2024 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

lantoli Oct 14, 2024 • 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

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

lantoli Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

maastha Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Oct 14, 2024 • 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

AgustinBettati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AgustinBettati Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EspenAlbert Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

lantoli Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

EspenAlbert left a comment

Choose a reason for hiding this comment

chore: Creates `advancedclustertpf` package for new implementation #2679

chore: Creates `advancedclustertpf` package for new implementation #2679

lantoli commented Oct 11, 2024 •

edited

Loading

lantoli Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024 •

edited

Loading

maastha Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024 •

edited

Loading

AgustinBettati Oct 14, 2024 •

edited

Loading

EspenAlbert Oct 14, 2024 •

edited

Loading

lantoli Oct 14, 2024 •

edited

Loading