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

Elasticsearch indices data source #686

Merged
merged 6 commits into from
Sep 11, 2024
Merged

Elasticsearch indices data source #686

merged 6 commits into from
Sep 11, 2024

Conversation

jdesnoes
Copy link
Contributor

Pull request to add an indices data source

@cla-checker-service CLA Checker Service
Copy link

cla-checker-service bot commented Jul 19, 2024 •
edited
Loading

💚 CLA has been signed

@tobio
Copy link
Member

tobio commented Aug 8, 2024

@jdesnoes are you able to take a look at the CLA so we can move forward with this?

@jdesnoes
Copy link
Contributor Author

@jdesnoes are you able to take a look at the CLA so we can move forward with this?

Hello @tobio, I already signed the CLA the 18th of July, with my GitHub's username and email.
But it still looks unchecked on GitHub. And I just signed it again now but same.
Can you take a look? Thanks.

@tobio
Copy link
Member

tobio commented Aug 12, 2024

But it still looks unchecked on GitHub

I can see you've signed the CLA, however the email address used to author the commits here isn't associated with your GH account at all. If you can re-author the commits with the email shared as part of the CLA process (@so*ria.com) or any email that's linked to your GH account then it'll get matched up properly.

@jdesnoes jdesnoes force-pushed the elasticsearch-indices-data-source branch from e46cbbc to df2eb91 Compare August 16, 2024 15:55
@jdesnoes
Copy link
Contributor Author

But it still looks unchecked on GitHub

I can see you've signed the CLA, however the email address used to author the commits here isn't associated with your GH account at all. If you can re-author the commits with the email shared as part of the CLA process (@so*ria.com) or any email that's linked to your GH account then it'll get matched up properly.

Thank you, that was the problem indeed, I fixed it and the CLA now appears signed.

@jdesnoes jdesnoes force-pushed the elasticsearch-indices-data-source branch from db27dc8 to e5bb843 Compare August 23, 2024 08:27
@jdesnoes
Copy link
Contributor Author

Hello @tobio
I updated my PR with the last changes of the main branch.
Can you take a look and let me know if it's valid to be merged?
Thank you.

@tobio
Copy link
Member

tobio commented Aug 26, 2024

@jdesnoes sorry, I've been meaning to look at this in comparison to #698 which moves the existing index resource to the plugin framework being used here. We should be able to re-use a lot of the model code in both of these PR's.

I think it makes sense to get this merged and then I can merge the two code bases together. Just a heads up, I think it makes sense to get both of these merged quite close together. This one may sit approved but unmerged for a little while whilst I get them both inline.

The actual code changes look good, thanks for adding this! Are you able to take a look at why the tests are failing?

@jdesnoes
Copy link
Contributor Author

@jdesnoes sorry, I've been meaning to look at this in comparison to #698 which moves the existing index resource to the plugin framework being used here. We should be able to re-use a lot of the model code in both of these PR's.

I think it makes sense to get this merged and then I can merge the two code bases together. Just a heads up, I think it makes sense to get both of these merged quite close together. This one may sit approved but unmerged for a little while whilst I get them both inline.

The actual code changes look good, thanks for adding this! Are you able to take a look at why the tests are failing?

Thanks for your answer @tobio

Indeed, considering the #698 PR purpose, there is some code that can be used in common with this current PR, mostly for the model.
Considering that, and to make the merge between both of those pull requests easier, I already updated a bit my code to make it more similar to yours, mainly the models and the schema.

I also fixed the unit tests.

So that, no problem if in the meanwhile this PR has to be sit approved but unmerged.

@tobio
Copy link
Member

tobio commented Aug 28, 2024

Thanks mate, we're working through the review process on the other one now, so we should get the both merged in the next couple of weeks.

tobio
tobio reviewed Aug 28, 2024
View reviewed changes
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.

Thanks for your patience on this one. One small change on the schema, where I think keeping with the documented names may be nicer. Otherwise this LGTM.

internal/elasticsearch/index/indices/schema.go Outdated Show resolved Hide resolved
@jdesnoes
Copy link
Contributor Author

Thanks again for your help and your review @tobio

You're definitely right, it makes sense to keep the name and the description of the official Elasticsearch API documentation.
Indeed, target looks the best option to cover all the cases as the search field name.

I just updated the pull request to take this in account, thanks.

@tobio
Merge remote-tracking branch 'origin/main' into elasticsearch-indices…
fa6c403 
…-data-source

* origin/main: (23 commits)
  Migrate index resource to plugin framework (elastic#698)
  Remove duplicate code (elastic#761)
  fix(deps): update module github.com/hashicorp/terraform-plugin-framework-jsontypes to v0.2.0 (elastic#760)
  fix(deps): update module github.com/golangci/golangci-lint to v1.61.0 (elastic#759)
  Add support for the`frequency` field in the Create Rule API (elastic#753)
  Prevent a provider panic when the referenced snapshot repo does not exist (elastic#758)
  remove space_id paramter from private locations (elastic#733)
  chore(deps): update dependency go to v1.23.1 (elastic#755)
  chore(deps): update docker.elastic.co/elasticsearch/elasticsearch docker tag to v8.15.1 (elastic#756)
  chore(deps): update golang:latest docker digest to 4a3c2bc (elastic#754)
  chore(deps): update docker.elastic.co/kibana/kibana docker tag to v8.15.1 (elastic#757)
  Fixup error handling during saved object imports (elastic#738)
  fix(deps): update module github.com/goreleaser/goreleaser to v2 (elastic#748)
  chore(deps): update golangci/golangci-lint-action action to v6 (elastic#746)
  chore(deps): update codecov/codecov-action action to v4 (elastic#745)
  fix(deps): update module github.com/go-resty/resty/v2 to v2.14.0 (elastic#741)
  chore(deps): update actions/setup-go action to v5 (elastic#744)
  chore(deps): update actions/checkout action to v4 (elastic#743)
  fix(deps): update module github.com/deepmap/oapi-codegen/v2 to v2.3.0 (elastic#740)
  chore(deps): update dependency go to v1.23.0 (elastic#739)
  ...
@tobio
Copy link
Member

tobio commented Sep 11, 2024

I'll look at merging the duplicate code in a follow up PR. Thanks for taking the time to add this one in @jdesnoes!

@tobio tobio merged commit 17859f3 into elastic:main Sep 11, 2024
20 checks passed
daemitus added a commit to daemitus/terraform-provider-elasticstack that referenced this pull request Sep 11, 2024
@daemitus
Revert "Elasticsearch indices data source (elastic#686)"
f033912 
This reverts commit 17859f3.
daemitus added a commit to daemitus/terraform-provider-elasticstack that referenced this pull request Sep 11, 2024
@daemitus
Revert "Elasticsearch indices data source (elastic#686)"
d34d5e7 
This reverts commit 17859f3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tobio tobio tobio approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@jdesnoes @tobio