Skip to content

Conversation

daemitus
Copy link
Contributor

The idea here is to take the work put into the Fleet OAS schema, and start preparing to migrate the Kibana APIs to it as they become available in the elastic/kibana oas folder.

  • Delete the generated/data_views spec, pull data_views from kibana instead (as a start). On this note, the data_views subset of the schema still appears to be created from a bundled file, rather than auto-generated. The schema needed minimal modifications to support what was being already being done. I can try creating an issue upstream for those like before.
  • Create a kibana2 client and config for lack of a better name. Once libs/go-kibana-rest is no longer needed, just rename the whole mess to kibana. I imagine there will be some discussion on this. Most of it is just copied from the Fleet client.
  • At some point way after all this, merge the generated fleet and kibana files since they're pulling the same spec down.
  • Reworked the kibana_dataviews resource. Was using this two stage tfsdk model and regular type model combo that is a bit overkill.

Notable tidbits for the resource:

  • Computed, Optional fields with no default set can have unknown values, for strings this is Pointer(""). So using somefield.ValueStringPointer() ends up sending a value that is not desired to the API. Added a helper for it. Surprisingly this did not crop up in the Fleet APIs as they all have defaults.
  • Existing functionality allows for semanticEqual between null configs and empty results for the SourceFilters, FieldAttrs, RuntimeFieldMap, and FieldFormats fields. Defaults could be set to be empty maps/slices, or keep the functionality I migrated.
  • There was logic for field_attrs.count being int or float from the API that I need to verify. right now its set to int only. The current docs say integer here

@tobio
Copy link
Member

tobio commented Nov 3, 2024

It looks like the operationId field has been fixed up with elastic/kibana#198132. The published docs now include this change which should mean we can rip the operationId re-writing out of this code.

@daemitus
Copy link
Contributor Author

daemitus commented Nov 3, 2024 via email

@daemitus
Copy link
Contributor Author

Pulled a fresh copy again, the Delete EPM package body (force) was removed, apparently it was deprecated the entire time. Along with a bunch of kafka topic bits that aren't implemented here yet. Also included some import formatting and a golangci-lint finding in the factory_test file.

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.

kibana2

I don't feel incredibly strongly here. There's benefits to using this naming in that we'll definitely rename it when the OG kibana client is removed.

Let's assume this exists in the codebase for 'a while' though which doesn't seem unreasonable. I'd prefer something more descriptive for example kibana_oapi or gen_kibana, at least then it's obvious what the difference between the clients is, though maybe less obvious which to choose for new code.

I'm happy to leave it as is if you do feel strongly here. I hate it, but that's not necessarily a bad thing :)

@daemitus
Copy link
Contributor Author

kibana2

I don't feel incredibly strongly here. There's benefits to using this naming in that we'll definitely rename it when the OG kibana client is removed.

Let's assume this exists in the codebase for 'a while' though which doesn't seem unreasonable. I'd prefer something more descriptive for example kibana_oapi or gen_kibana, at least then it's obvious what the difference between the clients is, though maybe less obvious which to choose for new code.

I'm happy to leave it as is if you do feel strongly here. I hate it, but that's not necessarily a bad thing :)

changed to kibana_oapi. I played around with merging them together and some other ideas, which only made the spaghetti worse.

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. Thanks for this one. Sorry about the review taking a little longer.

@tobio tobio merged commit 4e803b2 into elastic:main Nov 25, 2024
20 checks passed
@daemitus
Copy link
Contributor Author

daemitus commented Dec 3, 2024

No problem, its a big architectural change after all. 👍

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