-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(ingest): support writing configs to files #8696
Conversation
Adds the `__DATAHUB_TO_FILE_` directive to our config loader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing context on how we intend to use this feature. If this is just to help us run UI ingestions, this seems good. If we're going to suggest others do this, I could see it being confusing and would prefer generating these files in the relevant ConfigModel class rather than having users specify this behavior
@@ -67,12 +68,40 @@ def list_referenced_env_variables(config: dict) -> Set[str]: | |||
return set([call[1][0] for call in calls]) | |||
|
|||
|
|||
WRITE_TO_FILE_DIRECTIVE_PREFIX = "__DATAHUB_TO_FILE_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this notion of "directive" a yaml thing or something we've introduce before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's part of "datahub-flavored yaml", which is something I just made up :)
this change is mainly to make it easy to run UI ingestion - don't expect this to be a common way we do things broadly
Adds the
__DATAHUB_TO_FILE_
directive to our config loader.Some sources (e.g. kafka, bigquery) require paths to files on a local file system. This doesn't work for UI ingestion, where the container needs to be totally self-sufficient.
Checklist