fix: Fixes `network_container` resource update #2055

lantoli · 2024-03-21T05:25:36Z

Description

Fixes network_container resource update when only one field changes.

Based on this PR: #2031

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.
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

lantoli · 2024-03-21T06:28:33Z

thanks @BrettFolkins for your contribution, this PR is based on yours

lantoli · 2024-03-21T06:43:03Z

internal/service/networkcontainer/resource_network_container.go

@@ -51,6 +50,7 @@ func Resource() *schema.Resource {
 				Type:         schema.TypeString,
 				Optional:     true,
 				Default:      constant.AWS,
+				ForceNew:     true,


provider change forces a replace

Just to understand the why behind this change, the API does not support updating the providerName?

i was under that impression but i checked again and it looks like it's allowed (although it's a bit tricky because they need different attributes).
updated

lantoli · 2024-03-21T06:43:58Z

internal/service/networkcontainer/resource_network_container.go

-	if d.HasChange("region") {
-		region, _ := conversion.ValRegion(d.Get("region"))
-		container.Region = &region
+	if regionList, ok := d.GetOk("regions"); ok {


regions was not updated before

lantoli · 2024-03-21T06:44:33Z

internal/service/networkcontainer/resource_network_container_test.go

@@ -24,7 +24,7 @@ func TestAccNetworkContainer_basicAWS(t *testing.T) {
 		projectID        = acc.ProjectIDExecution(t)
 		randInt          = acctest.RandIntRange(0, 255)
 		cidrBlock        = fmt.Sprintf("10.8.%d.0/24", randInt)
-		randIntUpdated   = acctest.RandIntRange(0, 255)
+		randIntUpdated   = (randInt + 1) % 256


to make sure cidr and updated cidr are always different

marcosuma · 2024-03-21T08:00:03Z

internal/service/networkcontainer/resource_network_container.go

@@ -211,40 +211,33 @@ func resourceRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Di
 }

 func resourceUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
+	if !d.HasChange("atlas_cidr_block") && !d.HasChange("region_name") && !d.HasChange("region") && !d.HasChange("regions") {


missing provider_name?

no because I've set it to ForceNew, so if it changes it'll delete and create instead of update

marcosuma · 2024-03-21T08:02:42Z

internal/service/networkcontainer/resource_network_container.go

+	regionName, _ := conversion.ValRegion(d.Get("region_name"))
+	region, _ := conversion.ValRegion(d.Get("region"))
+
+	params := &admin.CloudProviderContainer{


One behaviour change compared to before is that we're going to pass all the values no matter if they have changed or not (As long as at least one has changed) - why are we changing this behaviour?

it's same behavior, we're not really passing all values, only the ones that are set, it will pass nil if the string is empty so it won't go as now (because StringPtr).
and for the values that are set, the change is precisely to pass all of them because the API will fail if we don't

From #2031 it would seem that regionName is the only property that must be included in the update. For the rest we can include them only if they have mainly for consistency with how we handle other attributes in this same function.

if you change the provider name you might want to change other attributes as different providers require different attributes. i've change it to send the require argument depending on the provider. It's the same idea as we're doing in Create

looping in @BrettFolkins for this just for awareness

I'm not familiar enough with best practices in terraform providers to have an opinion about sending the request with only the necessary changes or the full target state, but this version lgtm. Thanks!

lantoli · 2024-03-21T09:26:52Z

internal/service/networkcontainer/resource_network_container.go

 		if err != nil {
 			return diag.FromErr(fmt.Errorf(errorContainerUpdate, containerID, err))
 		}
+		params.SetRegionName(regionName)
+	case constant.AZURE:
+		region, err := conversion.ValRegion(d.Get("region"))


checking errors, it was not done before

EspenAlbert · 2024-03-21T11:10:10Z

internal/service/networkcontainer/resource_network_container_test.go

+					resource.TestCheckResourceAttrSet(resourceName, "project_id"),
+					resource.TestCheckResourceAttr(resourceName, "provider_name", constant.AWS),
+					resource.TestCheckResourceAttrSet(resourceName, "provisioned"),


These lines seems to be repeated many times. Maybe refactor into a helper?

i think there was a similar comment in another PR, I don't disagree but let's talk to use the same approach in all resources

marcosuma · 2024-03-21T12:56:12Z

internal/service/networkcontainer/resource_network_container.go

@@ -211,42 +210,49 @@ func resourceRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Di
 }

 func resourceUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics {
+	if !d.HasChange("region_name") && !d.HasChange("atlas_cidr_block") && !d.HasChange("region_name") && !d.HasChange("region") && !d.HasChange("regions") {


I think region_name is repeated twice? and missing provider_name? Sorry if I am missing some further context.

good catch, thanks! i was doing multiple tests and at the end i didn't revert this, updated

marcosuma · 2024-03-21T13:02:18Z

Let's make sure @BrettFolkins has the opportunity to review this PR (already sent a Slack DM)

BrettFolkins

LGTM, thanks for picking up where I left off and getting a full solution to this ready so quickly!

BrettFolkins · 2024-03-21T16:55:22Z

internal/service/networkcontainer/resource_network_container.go

+	regionName, _ := conversion.ValRegion(d.Get("region_name"))
+	region, _ := conversion.ValRegion(d.Get("region"))
+
+	params := &admin.CloudProviderContainer{


I'm not familiar enough with best practices in terraform providers to have an opinion about sending the request with only the necessary changes or the full target state, but this version lgtm. Thanks!

lantoli · 2024-03-21T17:10:34Z

internal/service/networkcontainer/resource_network_container_test.go

-					resource.TestCheckResourceAttrSet(dataSourcePluralName, "results.0.provider_name"),
-					resource.TestCheckResourceAttrSet(dataSourcePluralName, "results.0.provisioned"),
-				),
+				Check:  resource.ComposeTestCheckFunc(commonChecks(constant.AWS)...),


@EspenAlbert at then end i did the refactor you recommended following @AgustinBettati example in search_deployment. a lot of duplication gone and clearer to read. (still this resource might have more specific attribute checks, not only that they exist). cc @marcosuma

* failing test * recreate resource if provider_name changes * fix update * allow update in place when provider_name changes * fix regions error handling * refactor tests checks * refactor checks in mig tests as well * fix change check

…ted (#2052) * schedule compatibility matrix first day of month * WIP: test if status can be retrieved * test * test echo job status * change * Revert "change" This reverts commit 4b0e49e. * Revert "test echo job status" This reverts commit 4a83862. * test getting outputs of matrix * result * send slack * checkout step * fix name * correct json * formating * Revert "formating" This reverts commit f584822. * format * try * Revert "try" This reverts commit 7029f54. * use cat * Revert "use cat" This reverts commit 9503cd2. * remove unnecesary double quote * try cat * Revert "try cat" This reverts commit a2df8ff. * return json * unify json * fix * enrich message * TEMP: tag oncall on success * chore: Updates Atlas Go SDK (#2044) * build(deps): bump go.mongodb.org/atlas-sdk * fix tags * undo changes to admin20231001002 * API breaking change fixes --------- Co-authored-by: lantoli <[email protected]> * chore: Follow-up to Atlas SDK upgrade (#2051) * remove unused NewGroup * unify List call thanks to new API * refactor: Uses mocks on admin.APIClient for Project+Teams+ClustersAPIs instead of custom service (#2045) * refactor: Uses mocks on admin.APIClient instead of custom ClusterService (#2056) * fix: Fixes `network_container` resource update (#2055) * failing test * recreate resource if provider_name changes * fix update * allow update in place when provider_name changes * fix regions error handling * refactor tests checks * refactor checks in mig tests as well * fix change check * use MONGODB_ATLAS_PROJECT_ID (#2060) * chore: Removes old service from mockery (#2063) * feat: Allows user to specify to use an existing tag for release (#2053) * remove TEMP oncall tag on success * initialize oncall_tag * send notification when test suite fails * revert back testing change * remove temporary trigger of action * use oncall tag from secrets * Revert "use oncall tag from secrets" This reverts commit 5a7944f. * use SHA * new line * make oncall tag a secret --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: lantoli <[email protected]> Co-authored-by: Espen Albert <[email protected]> Co-authored-by: maastha <[email protected]>

failing test

6fe9bb9

github-actions bot added the bug label Mar 21, 2024

lantoli added 2 commits March 21, 2024 06:30

recreate resource if provider_name changes

d1f89f5

fix update

e363fe0

lantoli marked this pull request as ready for review March 21, 2024 06:27

lantoli requested a review from a team as a code owner March 21, 2024 06:27

lantoli commented Mar 21, 2024

View reviewed changes

marcosuma reviewed Mar 21, 2024

View reviewed changes

allow update in place when provider_name changes

0f73524

lantoli commented Mar 21, 2024

View reviewed changes

EspenAlbert reviewed Mar 21, 2024

View reviewed changes

EspenAlbert approved these changes Mar 21, 2024

View reviewed changes

marcosuma reviewed Mar 21, 2024

View reviewed changes

fix regions error handling

a6b5cd3

BrettFolkins approved these changes Mar 21, 2024

View reviewed changes

refactor tests checks

cae8e5d

lantoli commented Mar 21, 2024

View reviewed changes

lantoli added 2 commits March 21, 2024 18:30

refactor checks in mig tests as well

4de7e72

fix change check

2945134

lantoli merged commit 39ace4e into master Mar 21, 2024
43 of 45 checks passed

lantoli deleted the network_container_region_name branch March 21, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fixes `network_container` resource update #2055

fix: Fixes `network_container` resource update #2055

lantoli commented Mar 21, 2024 •

edited

Loading

lantoli commented Mar 21, 2024

lantoli Mar 21, 2024

AgustinBettati Mar 21, 2024

lantoli Mar 21, 2024 •

edited

Loading

lantoli Mar 21, 2024

lantoli Mar 21, 2024

marcosuma Mar 21, 2024

lantoli Mar 21, 2024

marcosuma Mar 21, 2024

lantoli Mar 21, 2024 •

edited

Loading

AgustinBettati Mar 21, 2024

lantoli Mar 21, 2024

marcosuma Mar 21, 2024

BrettFolkins Mar 21, 2024

lantoli Mar 21, 2024

EspenAlbert Mar 21, 2024

lantoli Mar 21, 2024

marcosuma Mar 21, 2024

lantoli Mar 21, 2024

marcosuma commented Mar 21, 2024

BrettFolkins left a comment

BrettFolkins Mar 21, 2024

lantoli Mar 21, 2024

fix: Fixes network_container resource update #2055

fix: Fixes network_container resource update #2055

Conversation

lantoli commented Mar 21, 2024 • edited Loading

Description

Type of change:

Required Checklist:

Further comments

lantoli commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lantoli Mar 21, 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

lantoli Mar 21, 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

Choose a reason for hiding this comment

marcosuma commented Mar 21, 2024

BrettFolkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix: Fixes `network_container` resource update #2055

fix: Fixes `network_container` resource update #2055

lantoli commented Mar 21, 2024 •

edited

Loading

lantoli Mar 21, 2024 •

edited

Loading

lantoli Mar 21, 2024 •

edited

Loading