feat: search node management with mongodbatlas_search_deployment resource and data source
#1633
Conversation
This reverts commit c556c7d.
| go-version-file: 'go.mod' | ||
| - name: Acceptance Tests | ||
| env: | ||
| MONGODB_ATLAS_PUBLIC_KEY: ${{ secrets.MONGODB_ATLAS_PUBLIC_KEY_CLOUD_DEV }} |
There was a problem hiding this comment.
CI: true is missing ;-). cc @marcosuma
There was a problem hiding this comment.
true, however that env variable is not used in this job as no tests need to be skipped (several jobs dont define it). Can add just for consistency.
There was a problem hiding this comment.
yeah, i know it's only used when calling SkipTestForCI , it was more to advocate for a common env zone :-)
There was a problem hiding this comment.
will try to answer to your PR :) but I think the global env variables would make the opposite effect here when you create a new job and you don't immediately expect there are global variables that might affect this job's behaviour
There was a problem hiding this comment.
or it could be the opposite, that you try to use SkipTestForCI and don't know why it's not skipping it.
I think it depends on the expectations set on the tests run from that GH action. for me it's clear in this case that all tests "want" to have CI:true even if they don't use/need it right now.
| updateTimeout, diags := searchDeploymentPlan.Timeouts.Update(ctx, defaultSearchNodeTimeout) | ||
| resp.Diagnostics.Append(diags...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } |
There was a problem hiding this comment.
I see this is more or less the same that is in create any chance we could make this method leaner and move this inside the waitSearch...? or just make it a separate method with a one-liner call here..
There was a problem hiding this comment.
So there are some details here to take into account:
- for the simple approach of extracting to separate methods and having a one-liner: the part checking if there are errors and finish the update function (L193-L195) can not be extracted. L192 could potentially be extracted into separate function, but not in favour of passing resp and mutating within another function (harder to follow).
- for having the logic inside waitSearch function, an additional parameter would be added to receive the function
searchDeploymentPlan.Timeouts.CreateandsearchDeploymentPlan.Timeouts.Updatewhich add some complexity to the method signature. This is why I end up passingtimeout time.Durationdirectly with some common code in each method.
There was a problem hiding this comment.
moving forward with the merge, if any further action comes from this comment we can address in a followup PR.
| deploymentResp, resp, err := client.AtlasSearchApi.GetAtlasSearchDeployment(ctx, projectID, clusterName).Execute() | ||
| if err != nil && deploymentResp == nil && resp == nil { | ||
| return nil, "", err | ||
| } | ||
| if err != nil { | ||
| if resp.StatusCode == 400 && strings.Contains(err.Error(), searchDeploymentDoesNotExistsError) { | ||
| return "", retrystrategy.RetryStrategyDeletedState, nil | ||
| } | ||
| if resp.StatusCode == 503 { | ||
| return "", retrystrategy.RetryStrategyPendingState, nil | ||
| } | ||
| return nil, "", err | ||
| } | ||
|
|
||
| if util.IsStringPresent(deploymentResp.StateName) { | ||
| tflog.Debug(ctx, fmt.Sprintf("search deployment status: %s", *deploymentResp.StateName)) | ||
| return deploymentResp, *deploymentResp.StateName, nil | ||
| } | ||
| return deploymentResp, "", nil |
There was a problem hiding this comment.
ah, this could have been so perfectly unit tested... can't wait to see them 😛
Co-authored-by: Marco Suma <[email protected]>
|
Description
Jira tickets: INTMDB-1273, INTMDB-1257
Includes change from #1621 and #1609.
This PR is using the latest SDK release, and adjusting documentation links.
Type of change:
Required Checklist:
Further comments