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

feat(opensearch): implement support for opensearch acls #316

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

mortenlj
Copy link
Contributor

Fixes #315

@mortenlj mortenlj requested a review from a team October 13, 2023 11:51
@Serpentiel Serpentiel self-assigned this Oct 16, 2023
@Serpentiel
Copy link
Contributor

Serpentiel commented Oct 16, 2023

hey, @mortenlj! 👋

thank you for your contribution!

it is possible to manage the OpenSearch ACLs via the ElasticSearch API, have you tried to use it?

this is the way our Terraform provider manages them; I'm not sure if we would want to have duplicates of the similar endpoints

@mortenlj
Copy link
Contributor Author

it is possible to manage the OpenSearch ACLs via the ElasticSearch API, have you tried to use it?

No, didn't try as the Elasticsearch API isn't documented anywhere. I see now that it works.

I'm not sure if we would want to have duplicates of the similar endpoints

For what it's worth, keeping the client code for a product you no longer offer while not having client code for the product that has replaced it seems somewhat of a strange choice. 🙂
Especially since it would seem there is some kind of compatability-layer in the API to convert from opensearch to elasticsearch, which presumably is going to be removed at some point.

opensearch_acls.go Outdated Show resolved Hide resolved
opensearch_acls.go Show resolved Hide resolved
opensearch_acls.go Show resolved Hide resolved
@Serpentiel
Copy link
Contributor

also, I suggest that you update ElasticSearch part of the client and mark the methods as deprecated

@Serpentiel
Copy link
Contributor

hmm, looks like commitlint has failed on this PR because of your 2nd commit; could you perhaps squash the commits in the single, original one? thank you!

@mortenlj mortenlj force-pushed the opensearch-acl branch 2 times, most recently from ce918f2 to 77bd795 Compare October 17, 2023 08:43
@mortenlj
Copy link
Contributor Author

hmm, looks like commitlint has failed on this PR because of your 2nd commit; could you perhaps squash the commits in the single, original one? thank you!

Tried doing that, only to now realize I've picked up a random dependabot commit on the way. Not sure how to undo that, my git fu isn't that strong ...

@mortenlj mortenlj requested a review from Serpentiel October 17, 2023 09:13
@Serpentiel
Copy link
Contributor

hey, @mortenlj! 👋

I could fix it for you if you allow edits of this PR for maintainers in the PR

@mortenlj
Copy link
Contributor Author

I could fix it for you if you allow edits of this PR for maintainers in the PR

I couldn't find that option, but got some help with the git commands and managed to fix it I believe.

Copy link
Contributor

@Serpentiel Serpentiel left a comment

Choose a reason for hiding this comment

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

lgtm

@Serpentiel Serpentiel merged commit cef6f95 into aiven:main Oct 18, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support OpenSearch ACLs
2 participants