-
Notifications
You must be signed in to change notification settings - Fork 211
Support overriding defaults.yml
based on the directory hierarchy
#500
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
Conversation
087669a
to
27a2bca
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #500 +/- ##
==========================================
+ Coverage 92.35% 92.54% +0.18%
==========================================
Files 11 11
Lines 1099 1140 +41
==========================================
+ Hits 1015 1055 +40
- Misses 84 85 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
defaults.yml
arguments using directory hierarchydefaults.yml
based on the directory hierarchy
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.
LGTM. Happy with the implementation. Couple of in-line comments (a quick question and a suggestion).
Introduce support to multiple
defaults.yaml
files, allowing properties to be overridden following the proximity to the YAML DAG definition.Use predictable merge behaviour where the level closest to the DAG YAML files wins. Keep the default section in-DAG for fine-grained overrides and as a template pattern for scoped DAGs.
defaults.yml
not only acceptsdefault_args
, but also DAG-level arguments;defaults.yml
per folder, and merging it with the nearest one will get more priority;defaults.yml
configurations merged.Closes: #469
The documentation was added in #504.
There are some open questions:
default_args_config_path
anddefault_args_config_dict
support overriding not onlydefault_args
, but also any DAG-level args, we should rename them. I've logged a follow-up ticket: Renamedefault_args_config_path
anddefault_args_config_dict
#505default_args_config_path
anddefault_args_config_dict
are exposed to users that leverageDagFactory(...).generate_dags()
. If we intend to deprecate this method, should we expose these fields viaload_yaml_dags
?