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

Target config extended configuration #945

Merged
merged 10 commits into from
Nov 4, 2024
Merged

Conversation

pdelewski
Copy link
Contributor

@pdelewski pdelewski commented Nov 4, 2024

This PR implements new variant of target configuration where it's a list not an array and can have more attributes (potentially in the future)

Current variant is still supported and is a fallback in the case extended version is missing

Below

  kibana_sample_data_flights:
    target:
    - my-clickhouse-data-source:
        useCommonTable: true

is equivalent of

  kibana_sample_data_flights:
    target: [my-clickhouse-data-source]
    useCommonTable: true

PR contains a bit of duplicated code, something that I would prefer fix in next PR.

@pdelewski pdelewski changed the title Target config extended initial commit Target config extended configuration Nov 4, 2024
@pdelewski pdelewski marked this pull request as ready for review November 4, 2024 13:38
@pdelewski pdelewski requested a review from a team as a code owner November 4, 2024 13:38
useCommonTable: false
kibana_sample_data_flights:
target:
- my-clickhouse-data-source: {}
Copy link
Member

Choose a reason for hiding this comment

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

Is there some way to avoid having to write {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. It seems to be yaml syntax specific

Copy link
Member

@avelanarius avelanarius left a comment

Choose a reason for hiding this comment

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

The new configuration syntax looks OK, but I think this looks strange:

        kibana_sample_data_flights:
          target:
            - my-clickhouse-data-source: {}

and it'd be great if something like this was possible instead:

        kibana_sample_data_flights:
          target:
            - my-clickhouse-data-source

@pdelewski
Copy link
Contributor Author

pdelewski commented Nov 4, 2024

The new configuration syntax looks OK, but I think this looks strange:

        kibana_sample_data_flights:
          target:
            - my-clickhouse-data-source: {}

and it'd be great if something like this was possible instead:

        kibana_sample_data_flights:
          target:
            - my-clickhouse-data-source

Let me check that again. Ok, this seems to be working. I think I made some mistake previously. I will update config

@pdelewski pdelewski added this pull request to the merge queue Nov 4, 2024
Merged via the queue into main with commit 5d19d94 Nov 4, 2024
5 checks passed
@pdelewski pdelewski deleted the target-config-extended branch November 4, 2024 16:12
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