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.

Sign up for GitHub

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

Jump to bottom

fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource #1950

Merged
merged 5 commits into from Feb 16, 2024
Merged

fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource #1950

merged 5 commits into from Feb 16, 2024

Conversation

maastha
Copy link
Collaborator

@maastha maastha commented Feb 15, 2024 •
edited

Description

mongodbatlas_search_index resource currently triggers deletion if update times out/fails. This PR aims to:

  • Remove deletion logic upon update failure as users may lose essential functionality/data upon updates
  • Updates error messaging to convey above change in behavior.

Link to any related issue(s): https://jira.mongodb.org/browse/CLOUDP-229877

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

New error message upon UPDATE failure:
Screenshot 2024-02-16 at 11 07 47

@maastha maastha changed the title fix: fix timeout and deletion on update fix: Fixes timeout and deletion on update Feb 15, 2024
@github-actions github-actions bot added the bug label Feb 15, 2024
@maastha maastha changed the title fix: Fixes timeout and deletion on update fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource Feb 16, 2024
@maastha maastha changed the title fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource Feb 16, 2024
@maastha maastha changed the title fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource Feb 16, 2024
@maastha maastha marked this pull request as ready for review February 16, 2024 12:38
@maastha maastha requested a review from a team as a code owner February 16, 2024 12:38
lantoli
lantoli reviewed Feb 16, 2024
View reviewed changes
internal/service/searchindex/resource_search_index.go
d.SetId("")
return diag.FromErr(fmt.Errorf("error creating index in cluster (%s): %s", clusterName, err))
Copy link
Member

Choose a reason for hiding this comment

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

if we don't delete it, should we keep the id and don't set it to "" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice catch! fixed

marcosuma
marcosuma reviewed Feb 16, 2024
View reviewed changes
internal/service/searchindex/resource_search_index.go
resourceDelete(ctx, d, meta)
d.SetId("")
return diag.FromErr(fmt.Errorf("error creating index in cluster (%s): %s", clusterName, err))
return diag.FromErr(fmt.Errorf("error updating index in cluster (%s). mongodbatlas_search_index resource was not deleted : %s", clusterName, err))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why do we need to specify the resource was not deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is mostly for users who might be expecting the previous incorrect behavior of the index being deleted in case of errors/timeouts, see comment

@maastha maastha merged commit cee70ec into master Feb 16, 2024
44 checks passed
@maastha maastha deleted the CLOUDP-229877-search-idx-deletion-timeout branch February 16, 2024 14:33
lantoli added a commit that referenced this pull request Feb 16, 2024
@lantoli
Merge branch 'master' into CLOUDP-229554_advanced_cluster
ad8647b 
* master:
  fix: Fixes timeout and removes deletion logic on update failure for mongodbatlas_search_index resource (#1950)
  task: remove dependency on pointy (#1953)
  fix: Converts snapshot_id field from required to optional. (#1924)

# Conflicts:
#	internal/service/cluster/resource_cluster.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lantoli lantoli lantoli approved these changes

@marcosuma marcosuma marcosuma approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@maastha @lantoli @marcosuma