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

fix: Sets replication_specs IDs when updating them. #1876

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Conversation

marcosuma
Copy link
Collaborator

@marcosuma marcosuma commented Jan 26, 2024

Description

Passes the replicationSpec item id to the PATCH request during updates. This fixes the error:

400 (request "INVALID_CLUSTER_CONFIGURATION") The specified cluster configuration is not valid: Updating id of existing replication spec.

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

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

@marcosuma marcosuma changed the title fix: sets replication_specs IDs when updating them. fix: Sets replication_specs IDs when updating them. Jan 26, 2024
@github-actions github-actions bot added the bug label Jan 26, 2024
@marcosuma marcosuma marked this pull request as ready for review February 2, 2024 06:31
@marcosuma marcosuma requested a review from a team as a code owner February 2, 2024 06:31
lantoli
lantoli approved these changes Feb 2, 2024
Copy link
Member

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

LGTM

@@ -850,6 +850,10 @@ func expandAdvancedReplicationSpec(tfMap map[string]any) *matlas.AdvancedReplica
RegionConfigs: expandRegionConfigs(tfMap["region_configs"].([]any)),
}

if tfMap["id"].(string) != "" {
Copy link
Member

@AgustinBettati AgustinBettati Feb 2, 2024

Choose a reason for hiding this comment

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

Just to confirm, we are setting the id in update request of mongodbatlas_cluster resource right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AgustinBettati this method is called in both update and create. The reason why it should only run on the update is because of the if itself. I am assuming that at the creation phase this field is empty. Any concerns?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The function is called expandAdvancedReplicationSpec but this PR is also updating the cluster test 🤔 . Is expandAdvancedReplicationSpec called also in the cluster resource? if not, is the cluster resource already sending the id during the update?

Copy link
Member

Choose a reason for hiding this comment

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

Yes that statement seems correct to me. My doubt was more related to how we handle this id in mongodbatlas_cluster, not a blocker just something to keep in mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The function is called expandAdvancedReplicationSpec but this PR is also updating the cluster test
@andreaangiolillo @AgustinBettati

Adding tests to cluster was more like to increase test coverage. Sorry if it created confusion. confirming that expandAdvancedReplicationSpec is only affecting advanced_cluster.

regarding cluster: that is a completely different story: while in here we use list, in that one we are representing replication_specs as a set and we're also using v1.0 - I added the test on purpose: to see if it was able to handle replicationspecs updates and indeed it does without having to change the logic.

@@ -850,6 +850,10 @@ func expandAdvancedReplicationSpec(tfMap map[string]any) *matlas.AdvancedReplica
RegionConfigs: expandRegionConfigs(tfMap["region_configs"].([]any)),
}

if tfMap["id"].(string) != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The function is called expandAdvancedReplicationSpec but this PR is also updating the cluster test 🤔 . Is expandAdvancedReplicationSpec called also in the cluster resource? if not, is the cluster resource already sending the id during the update?

@marcosuma marcosuma merged commit cbeec93 into master Feb 2, 2024
57 checks passed
@marcosuma marcosuma deleted the HELP-54829 branch February 2, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants