merge main to feature/click-cli #6483
Conversation
ChenyuLInx
requested review from
iknox-fa,
peterallenwebb,
MichelleArk and
stu-k
and removed request for
a team,
leahwicz and
McKnight-42
| def setup_event_logger(log_path, log_format, use_colors, debug): | ||
| global FILE_LOG | ||
| global STDOUT_LOG | ||
| def _get_stdout_config(log_format: str, debug: bool, use_colors: bool) -> LoggerConfig: |
There was a problem hiding this comment.
@peterallenwebb I feel like I got to a sub optimal solution for this entire file to make mypy happy. Feel free to update or point me to the right direction
ChenyuLInx
force-pushed
the
cl/merge-main-to-click
branch
from
7e1dd7e to
9c47d4e
Compare
| @@ -708,7 +704,7 @@ def build_manifest_state_check(self): | |||
| vars_hash = FileHash.from_contents( | |||
| "\x00".join( | |||
| [ | |||
| getattr(config.args, "vars", "{}") or "{}", | |||
| str(getattr(config.args, "vars", "{}") or "{}"), | |||
There was a problem hiding this comment.
This is being modified to support new loaded args
core/dbt/task/deps.py
Outdated
| "deps", | ||
| self.project.hashed_name(), | ||
| self.config, | ||
| self.config.args, |
There was a problem hiding this comment.
It looks like this part is undoing the deps implementation on the feature branch: https://github.com/dbt-labs/dbt-core/pull/6260/files#diff-77c52d1632e496415c4b241f4eb7ced5e8f7c6cbe207955177b2e8c6c6a54227R58-R59
| # currently fire before logs can be configured by setup_event_logger(), we | ||
| # create a default configuration with default settings and no file output. | ||
| EVENT_MANAGER: EventManager = EventManager() | ||
| EVENT_MANAGER.add_logger( |
There was a problem hiding this comment.
Does this seems right?
| # Create second stdout logger to support test which want to know what's | ||
| # being sent to stdout. | ||
| # debug here is true because we need to capture debug events, and we pass in false in main | ||
| capture_config = _get_stdout_config(log_format, True, use_colors) |
There was a problem hiding this comment.
This hard coded DEBUG is for tests, is it okay?
There was a problem hiding this comment.
It seems to me that we could pass the value of debug here as well, rather than hard coding. And if the tests want the debug output they should execute dbt with the debug flag set to true. Does that seem wrong?
There was a problem hiding this comment.
There's a issue regarding we add a stdout logger above this that also uses the debug flag, if debug is being set to true then that logger will actually raise some error during running tests.
| @@ -211,7 +211,8 @@ def run_from_args(parsed): | |||
| if task.config is not None: | |||
| log_path = getattr(task.config, "log_path", None) | |||
| log_manager.set_path(log_path) | |||
| setup_event_logger(log_path or "logs", "json", False, True) | |||
| # WHY WE SET DEBUG TO BE TRUE HERE previously? | |||
There was a problem hiding this comment.
Changing one hard-code to another to make sure all tests passing. This file should be removed after feature branch merged
There was a problem hiding this comment.
Reviewing by opening a branch on top of this one with some minor tweaks to get integration tests passing: #6531. All tests are passing there with understandable changes (except for one adapter one, but that is fixed by changes on this branch), which makes me much more comfortable that we aren't introducing regressions to any new functionality from main in this merge.
Thank you so much for doing this merge @ChenyuLInx, it'll go a long way in reducing the cognitive overhead necessary for working on this feature branch!
resolves #6478
Merges all changes from main to feature branch for click