refactor: Use named collections for dictionaries #554
Conversation
tutoraspects/templates/aspects/jobs/init/clickhouse/init-clickhouse.sh
Outdated
Show resolved
Hide resolved
bmtcril
force-pushed
the
bmtcril/named_collections
branch
from
eae2eab to
0a450e0
Compare
tutoraspects/plugin.py
Outdated
| ( | ||
| "CLICKHOUSE_EXTRA_NAMED_COLLECTION_XML_CONFIG", | ||
| """ | ||
| <local_ch_xapi> | ||
| <host overridable="false">localhost</host> | ||
| <port overridable="false">{{ CLICKHOUSE_INTERNAL_NATIVE_PORT }}</port> | ||
| <database overridable="false">{{ ASPECTS_XAPI_DATABASE }}</database> | ||
| <user overridable="false">{{ ASPECTS_CLICKHOUSE_REPORT_USER }}</user> | ||
| <password overridable="false">{{ ASPECTS_CLICKHOUSE_REPORT_PASSWORD }}</password> | ||
| <secure overridable="false">{% if CLICKHOUSE_SECURE_CONNECTION %}1{% else %}0{% endif %}</secure> | ||
| </local_ch_xapi> | ||
| <local_ch_event_sink> | ||
| <host overridable="false">localhost</host> | ||
| <port overridable="false">{{ CLICKHOUSE_INTERNAL_NATIVE_PORT }}</port> | ||
| <database overridable="false">{{ ASPECTS_EVENT_SINK_DATABASE }}</database> | ||
| <user overridable="false">{{ ASPECTS_CLICKHOUSE_REPORT_USER }}</user> | ||
| <password overridable="false">{{ ASPECTS_CLICKHOUSE_REPORT_PASSWORD }}</password> | ||
| <secure overridable="false">{% if CLICKHOUSE_SECURE_CONNECTION %}1{% else %}0{% endif %}</secure> | ||
| </local_ch_event_sink> | ||
| """ | ||
| ), |
There was a problem hiding this comment.
This is a too large to be a setting, can we use a patch for it and template it?
Also, as Clickhouse resolves any conflicts we can just override any value here using the patch
There was a problem hiding this comment.
I did this to match the other ClickHouse settings (CLICKHOUSE_EXTRA_XML_CONFIG, CLICKHOUSE_EXTRA_USERS_XML_CONFIG) so that they're not being configured in two different ways. I can make all of those patches, but I think that requires people to create a plugin to override them instead of just changing a config value? Just making sure since we would probably need to document the two types of settings clearly to say how to override the different settings.
There was a problem hiding this comment.
It's also worth noting that CLICKHOUSE_EXTRA_USERS_XML_CONFIG in the current configuration doesn't work on k8s as well. I wasn't able to solve that problem as part of this.
There was a problem hiding this comment.
That's a too large configuration to be managed as a setting. I would prefer the patch
This allows us to change admin passwords and keep credentials out of the migrations.
bmtcril
force-pushed
the
bmtcril/named_collections
branch
from
85199fb to
c3c6ac6
Compare
|
We can't actually use this until CH Cloud supports named collections / 23.11 build. So I'm closing this in favor of the work to get dictionaries into dbt, which would also solve this problem. |
This allows us to change admin passwords and keep credentials out of the migrations.
Draft for now, until I have a chance to test against CH Cloud and Altinity, but I want to see what CI does.