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

Add support for remote_indicies to elasticstack_elasticsearch_security_role & elasticstack_kibana_security_role #723

Merged
merged 14 commits into from
Aug 26, 2024

Conversation

mholttech
Copy link
Contributor

@mholttech mholttech commented Aug 21, 2024

resolves #722 by adding remote_indices to elasticstack_kibana_security_role & elasticstack_elasticsearch_security_role

Copy link

cla-checker-service bot commented Aug 21, 2024

💚 CLA has been signed

@mholttech mholttech force-pushed the 722-add-remote-indices branch from 71d3232 to 9312ea2 Compare August 21, 2024 00:29
@tobio
Copy link
Member

tobio commented Aug 21, 2024

It looks like the build is failing because the committed docs don't match what would be generated. Are you able to run make docs-generate and commit the result?

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

The linked issue talks about the elasticsearch role resource (which is here). I'm not sure if adding it to the Kibana role is sufficient for you though?

internal/kibana/role.go Show resolved Hide resolved
@mholttech
Copy link
Contributor Author

Good catch. I didn't realize there was two seperate resources. Looks like the api for elasticstack_kibana_security_role is in preview but the work done is still valid. I'll expand the PR to include the intended resource and regenerate the docs.

@mholttech mholttech changed the title Add remote indices feature WIP: Add remote indices feature Aug 21, 2024
@mholttech
Copy link
Contributor Author

Everything should be in place for the elasticstack_kibana_security_role now. Still working on the elasticstack_elasticsearch_security_role

@mholttech
Copy link
Contributor Author

forgot about TestAccDataSourceKibanaSecurityRole, i've commented out the remote_indices for now, will add the version constraints tomorrow.

@mholttech mholttech force-pushed the 722-add-remote-indices branch from 27d2686 to 952183e Compare August 22, 2024 19:17
@mholttech mholttech changed the title WIP: Add remote indices feature Add remote indices feature Aug 22, 2024
@mholttech mholttech changed the title Add remote indices feature Add support for remote_indicies to elasticstack_elasticsearch_security_role & elasticstack_kibana_security_role Aug 22, 2024
@mholttech
Copy link
Contributor Author

@tobio This is ready for another review

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

Couple of minor comments. Generally this LGTM. Thanks so much for adding this! It's a decent chunk of code here. ❤️

internal/elasticsearch/security/role.go Outdated Show resolved Hide resolved
internal/elasticsearch/security/role.go Outdated Show resolved Hide resolved
internal/kibana/role.go Outdated Show resolved Hide resolved
internal/models/models.go Outdated Show resolved Hide resolved
@mholttech
Copy link
Contributor Author

Couple of minor comments. Generally this LGTM. Thanks so much for adding this! It's a decent chunk of code here. ❤️

Thanks! I've reviewed your feedback and now that i've learned how everything works and it's in a working state I'll attempt some of the suggested code cleanup in indices and remote_indices before we merge this.

@mholttech
Copy link
Contributor Author

@tobio I just pushed some code improvements based on your feedback.

Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

LGTM. This is a big chunk of code, thanks for adding this to the provider!

@tobio tobio merged commit cf53ad9 into elastic:main Aug 26, 2024
20 checks passed
@mholttech
Copy link
Contributor Author

@tobio when will the next version be released so we can begin leveraging this?

@tobio
Copy link
Member

tobio commented Sep 10, 2024

@mholttech I'm expecting these to be released ideally later this week, but by the end of next week at the latest.

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.

[Feature] Add support for remote_indices in elasticstack_elasticsearch_security_role
2 participants