Skip to content

Conversation

tobio
Copy link
Member

@tobio tobio commented Sep 12, 2023

Fixes #345

Kibana spaces shouldn't need Elasticsearch to be configured to work.

@tobio tobio requested a review from dimuon September 12, 2023 10:31
@tobio tobio self-assigned this Sep 12, 2023
dimuon
dimuon previously approved these changes Sep 13, 2023
Copy link
Contributor

@dimuon dimuon left a comment

Choose a reason for hiding this comment

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

LGTM 👍, it just needs changelog :)

@dimuon
Copy link
Contributor

dimuon commented Sep 13, 2023

Still, it looks like we also have to change the way kibana client is built.

@dimuon dimuon self-requested a review September 13, 2023 08:40
if diags.HasError() {
return diags
id := d.Id()
if strings.Contains(id, "/") {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a trivial check, but maybe it makes sense to create a helper function (e.g. IsCompositeId) next to func CompositeIdFromStr to keep the details of composite ID creation in one place.

@dimuon dimuon self-requested a review September 13, 2023 08:49
@tobio
Copy link
Member Author

tobio commented Sep 17, 2023

@dimuon I might defer this one until #343 is done since it will simplify the creation changes.

@tobio tobio force-pushed the kibana-space-no-es branch from 148d197 to f78706e Compare October 16, 2023 05:46
Copy link
Contributor

@dimuon dimuon left a comment

Choose a reason for hiding this comment

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

LGTM

@tobio tobio merged commit 9d620f2 into main Oct 16, 2023
@tobio tobio deleted the kibana-space-no-es branch October 16, 2023 11:08
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.

Non Elasticsearch resources should not require Elasticsearch configuration.
2 participants