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

[databases]: change Opensearch ism_history_max_docs type to int64 to … #737

Merged

Conversation

loosla
Copy link
Contributor

@loosla loosla commented Oct 4, 2024

…align with api

This PR changes a type for Opensearch ism_history_max_docs to int64

Because DigitalOcean API doesn't accept value that exceeds int64 max limit.

@loosla loosla marked this pull request as ready for review October 4, 2024 16:49
@loosla loosla requested a review from andrewsomething October 4, 2024 16:50
@bentranter
Copy link
Member

This changes a type on a struct field that was already released, so this is technically a breaking change. I'm not sure if it's better to apply this breaking change since it's unlikely that anyone is already using this, but I also think it's unlikely that anyone will set the value anything greater than max uint64 (which is 18,446,744,073,709,551,615).

Maybe instead, we can add a note about the limitation that this should only be positive numbers in the doc string, and add a validation to prevent negative numbers from being submitted to the API?

@andrewsomething
Copy link
Member

@bentranter I don't believe this has been part of a tagged release yet. OpensearchConfig was added in #729

@loosla
Copy link
Contributor Author

loosla commented Oct 4, 2024

@bentranter We check the value to be >= 1 on our API side.

curl -X PATCH   -H "Content-Type: application/json"   -H "Authorization: Bearer $DIGITALOCEAN_TOKEN"   -d '{"config": {"ism_history_max_docs": 0}}'   "https://api.digitalocean.com/v2/databases/d79e2e2b-3493-4523-9352-4ec9ace11b08/config"

{"message":"invalid 'user_config': invalid input for opensearch: 0 is less than the minimum of 1","id":"unprocessable_entity","request_id":"6edcbe86-3d6a-4242-b50e-4f072901a9ad"}

It wasn't released yet
Screenshot from 2024-10-04 13-17-15

@bentranter
Copy link
Member

@andrewsomething @loosla Ah sorry about that, my bad – I got the wrong commit SHA from the git blame for that field. This change LGTM then!

@loosla loosla merged commit dfe74ef into main Oct 4, 2024
8 checks passed
@loosla loosla deleted the alushnikova/change-opensearch-ism-history-max-docs-type branch October 4, 2024 17:55
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.

3 participants