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

Implement ImportState and more robust Update checks #40

Merged
merged 29 commits into from
Mar 4, 2025

Conversation

RaynorChavez
Copy link
Member

@RaynorChavez RaynorChavez commented Feb 25, 2025

  • We implement ImportState so users can import existing resources created outside of Terraform IaC
  • Also add checks for updating fields - if you try to update any field other than inference type, number of inferences, shards, and replicas, then we fail fast instead of sending the request to marqo only for it to fail after the operation is done

}

// Handle TextPreprocessing
if !reflect.DeepEqual(indexDetail.TextPreprocessing, go_marqo.TextPreprocessing{}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really need reflect.DeepEqual to check whether indexDetail.TextPreprocessing is an empty struct of primitives? Can you just if indexDetail.TextPreprocessing != (go_marqo.TextPreprocessing{}) {...?

Also, nit, it would be marginally more readable to swap the conditions, i.e. if indexDetail.TextPreprocessing == (go_marqo.TextPreprocessing{}) {model.Settings.TextPreprocessing = nil} else {...unpacking stuff...}

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea thanks

if !reflect.DeepEqual(indexDetail.ModelProperties, go_marqo.ModelProperties{}) {
model.Settings.ModelProperties = convertModelPropertiesToResource(&indexDetail.ModelProperties)
} else {
model.Settings.ModelProperties = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this already nil by default? Do you need to set it explicitly?

Copy link
Member Author

@RaynorChavez RaynorChavez Feb 25, 2025

Choose a reason for hiding this comment

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

We need to explicitly set it here to normalize the value for ModelProperties in all possible situations that might lead to its value becoming an empty dict {}

So for example

  1. API might return an empty dict (we don't but if we ever did it would get corrected here)
  2. initial state - If a user specifies empty model properties (say {}) in their configuration, the state should show nil rather than an empty object.
  3. Refresh - need to normalize 2 (the config file) everytime we refresh

@RaynorChavez RaynorChavez force-pushed the implement_import_state branch from 4331a93 to 593fdbf Compare February 27, 2025 15:21
@RaynorChavez RaynorChavez merged commit 8a121fc into main Mar 4, 2025
3 checks passed
@RaynorChavez RaynorChavez deleted the implement_import_state branch March 4, 2025 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants