Add --use-colors cli option #2622

rsenseman · 2020-07-10T15:39:15Z

resolves #2192

Description

Adds two new flags --use-colors and --no-use-colors to dbt run command to enable or disable log colorization from the command line. The motivation for this change is to enable running with colorized logs locally and running without colorized logs in production for easier log parsing by other tools.

Also renamed a duplicately named test in the test_main file.

PR for documentation: dbt-labs/docs.getdbt.com#286

Checklist

I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

Testing

Ran locally with all 4 combinations of profiles options use_colors: < true | false > and cli options nothing, --use-colors, and --no-use-colors to make sure overrides work as expected. Also added integrations tests to auto-test that these flags work. Having tests for both --use-colors and --no-use-colors will implicitly test that default overrides are working.

Ran tests with command docker-compose run test tox -e explicit-py36 -- -s -x -m profile_postgres test/integration/061_use_colors_tests/. Output: gist

Also ran make test-unit to hopefully make sure I didn't break anything already in place

Documentation follows from dbt-labs/dbt-core#2622

beckjake

Thanks for your contribution, @rsenseman! This looks great, just have a note on the particulars of the approach.

Can you reorganize this a bit? I don't want flags relying on anything else in dbt if we can avoid it. How about only having USE_COLORS in flags and making ui.use_colors/ui.do_not_use_colors() change flags.USE_COLORS? I think that will make the code a bit simpler, too.

Also, there's a test looking for warning text color that's failing, I'm not 100% sure why that would be.

CHANGELOG.md

core/dbt/flags.py

test/integration/061_use_colors_tests/models/do_nothing_then_fail.sql

test/integration/061_use_colors_tests/test_use_colors.py

test/integration/061_use_colors_tests/test_no_use_colors.py

rsenseman · 2020-07-22T23:13:51Z

Thanks for your contribution, @rsenseman! This looks great, just have a note on the particulars of the approach.

Can you reorganize this a bit? I don't want flags relying on anything else in dbt if we can avoid it. How about only having USE_COLORS in flags and making ui.use_colors/ui.do_not_use_colors() change flags.USE_COLORS? I think that will make the code a bit simpler, too.

Also, there's a test looking for warning text color that's failing, I'm not 100% sure why that would be.

Yeah I'll take a look and see if I can refactor a bit. The seemingly unnecessary code volume comes from working with the existing process by which the profile arguments are parsed and applied first, then the command line arguments are parsed and applied. So there has to be a mechanism to override the profile config which goes through the ui.use_colors function right now

beckjake · 2020-07-23T13:40:07Z

The changes look really good, and I've tested it out locally and it works ok!

I see some test failures that are probably related to the CLI arguments changes (use_colors being None, for example). You'll also need to fix the type signature on dbt.contracts.connection.UserConfigContract to allow use_colors to be Optional like UserConfig. And flake8 is upset about line length, as usual.

Finally, can you rebase this into dev/marian-anderson and force push it? I think something weird happened, perhaps a merge with our (long-dead...) master branch? I just can't imagine why else those merges would be there in the branch commit history.

rsenseman · 2020-07-23T15:38:32Z

I'm planning on getting the tests working this morning, then I'll rebase and push; when I was debugging, I accidentally merged master out of habit instead of dev/marian-anderson.

rsenseman · 2020-07-23T23:11:11Z

@beckjake Things are working and cleaned up a bit but I'm having some trouble with the tests. Almost all of the tests now reflect the new flag setting process and work as expected except I cannot get the tests in test/unit/test_main.py to work as expected. They are setup now how I think they should work but they don't do what I expect. Can you help me figure out why the use_colors() patch and call specification isn't aligning between the code and the reality? Here are the failures I can't resolve: https://gist.github.com/rsenseman/1b54c33eed93b71f66bc373cd1e1fe15

Aside from that, it looks like I introduced some weirdness to the Changelog when I rebased, I will clean that up. I am going to be away from my computer until Monday then I will take another look at this.

beckjake · 2020-07-24T18:22:49Z

Hi @rsenseman, happy to help out! It looks like a lot of the changelog weirdness is actually my fault from merging 0.17.1 (or something) in poorly, so thank you for fixing that. We should probably find a markdown linter and run it over the changelog file as part of tests, or something! I did notice that as part of the merge, some changelog entries disappeared, 0.17.0rc2 in particular.

The unit tests also appear to be passing now - is this an intermittent issue? I did see them failing when you originally posted that, but they were cleared up by the time I got to it.

I also kicked the integration tests again. Redshift looked like a transient failure possibly caused by an incomplete download. The postgres test is an issue with an overly-strict RPC test that is unfortunately testing some non-deterministic behavior, we probably need to tweak the test a bit to handle latency in CI environments.

Finally, can you do an interactive rebase and squash those commits? Right now there are are 36 commits, including a number of duplicates. It looks like maybe you rebased and then merged the resulting rebased code with the original instead of force-pushing, that's a common way to get duplicate merge commits like this. Ideally this would be only a few commits! Let me know if I can help with that process at all. If it makes your life easier, feel free to open a fresh PR rather than rebasing/re-pushing.

remove colorize logs language rename duplicate test name rename to something useful add cli override add tests rename to correct test number update changelog fix flake errors and code cleanup move feature to Features section of changelog link issues and add names to list of contributors Update test/integration/061_use_colors_tests/test_no_use_colors.py Co-authored-by: Jacob Beck <beckjake@users.noreply.github.com> simplify test query refactor properly set default update tests Update CHANGELOG.md added colorize-logs cli option remove colorize logs language rename duplicate test name rename to something useful add cli override add tests rename to correct test number update changelog fix flake errors and code cleanup move feature to Features section of changelog link issues and add names to list of contributors Update test/integration/061_use_colors_tests/test_no_use_colors.py Co-authored-by: Jacob Beck <beckjake@users.noreply.github.com> simplify test query refactor properly set default update tests remove rebase leftovers get rid of threads again Parse selectors This doesn't use hologram, as it needs more permissive schemas than what hologram provides. Added some unit tests Create selector config file, handle selector argument Added tests Added RPC and CLI support add changelog Compile assets as part of docs generate Release dbt v0.17.1 Bump version: 0.17.1 → 0.17.2a1 add environment variables for logging changelog fix Release dbt v0.17.2b1 Fix fast-fail not logging original error message Apply suggestions from code review Co-authored-by: Jacob Beck <beckjake@users.noreply.github.com> azure pipelines silently messing with $PATH again clean without a profile Update CHANGELOG.md Co-authored-by: Jeremy Cohen <jeremy@fishtownanalytics.com> Always close connections in release() Handle the fallout of closing connections in release - close() implies rollback, so do not call it - make sure to not open new connections for executors in single-threaded mode - logging cleanups - fix a test case that never acquired connections - to cancel other connections, one must first acquire a connection for the master thread - change a number of release() calls to rollback release vs rollback alter the tests so we can run rpc tests on snowflake only try to cancel open connections missed the snowflake rpc tests PR feedback: - fix conftest adainivalue_line calls - deprecate the release argument to execute_macro Release dbt v0.17.2rc1 add a test for cross-schema catalog generation, fix redshift to work for schema overrides fix a test with too-strict bounds, any <=2 value is ok here Docs site updates for 0.17.2: fix code block background fix the changelog Release dbt v0.17.2

rsenseman · 2020-07-30T18:28:21Z

@beckjake Well it looks like I made a huge mess rebasing and squashing, any suggestions for what to do next? Looks like opening a new PR may be the easiest path forward

beckjake · 2020-07-30T21:28:02Z

@rsenseman I think that's probably for the best at this point, unfortunately.

jtcohen6 · 2020-08-14T17:05:38Z

@rsenseman Do you have a chance to open a new PR in the next few days? We're planning to cut a release candidate of v0.18.0 soon

rsenseman · 2020-08-16T17:39:52Z

@jtcohen6 I'll open a new PR today and get things sorted out. Sorry for the delay; after I made a mess of this PR, I had to prioritize some other work.

rsenseman · 2020-08-16T21:49:17Z

@beckjake @jtcohen6 The new PR is read to roll, automated tests are running now: https://github.com/fishtown-analytics/dbt/pull/2708/files.

Closing this PR in favor of the new one

cla-bot bot added the cla:yes label Jul 10, 2020

rsenseman changed the title ~~added colorize-logs cli option~~ Added colorize-logs cli option Jul 10, 2020

rsenseman changed the title ~~Added colorize-logs cli option~~ Add --colorize-logs cli option Jul 10, 2020

rsenseman changed the title ~~Add --colorize-logs cli option~~ Add --use-colors cli option Jul 14, 2020

rsenseman marked this pull request as ready for review July 14, 2020 23:45

rsenseman added a commit to rsenseman/docs.getdbt.com that referenced this pull request Jul 14, 2020

Update run.md

23b1604

Documentation follows from dbt-labs/dbt-core#2622

rsenseman mentioned this pull request Jul 14, 2020

Update run.md to document new dbt run options --use-colors and --no-use-colors dbt-labs/docs.getdbt.com#286

Merged

beckjake suggested changes Jul 17, 2020

View reviewed changes

Update CHANGELOG.md

f6918f5

rsenseman force-pushed the enhancement/color_output_command_line_flag branch from 67e19f8 to c1c8a6a Compare July 23, 2020 23:03

rsenseman force-pushed the enhancement/color_output_command_line_flag branch from a6d3d42 to 2d88d3a Compare July 30, 2020 16:24

rsenseman mentioned this pull request Aug 16, 2020

Add --use-colors cli option (second attempt) #2708

Merged

4 tasks

rsenseman closed this Aug 16, 2020

rexledesma mentioned this pull request Mar 21, 2022

Add configuration to disable colors in dbt run output dagster-io/dagster#7150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --use-colors cli option #2622

Add --use-colors cli option #2622

rsenseman commented Jul 10, 2020 •

edited

Loading

beckjake left a comment

rsenseman commented Jul 22, 2020

beckjake commented Jul 23, 2020

rsenseman commented Jul 23, 2020

rsenseman commented Jul 23, 2020 •

edited

Loading

beckjake commented Jul 24, 2020

rsenseman commented Jul 30, 2020

beckjake commented Jul 30, 2020

jtcohen6 commented Aug 14, 2020

rsenseman commented Aug 16, 2020

rsenseman commented Aug 16, 2020

Add --use-colors cli option #2622

Add --use-colors cli option #2622

Conversation

rsenseman commented Jul 10, 2020 • edited Loading

Description

Checklist

Testing

beckjake left a comment

Choose a reason for hiding this comment

rsenseman commented Jul 22, 2020

beckjake commented Jul 23, 2020

rsenseman commented Jul 23, 2020

rsenseman commented Jul 23, 2020 • edited Loading

beckjake commented Jul 24, 2020

rsenseman commented Jul 30, 2020

beckjake commented Jul 30, 2020

jtcohen6 commented Aug 14, 2020

rsenseman commented Aug 16, 2020

rsenseman commented Aug 16, 2020

rsenseman commented Jul 10, 2020 •

edited

Loading

rsenseman commented Jul 23, 2020 •

edited

Loading