Skip to content
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

Simplified logging #532

Merged
merged 2 commits into from
Feb 10, 2018
Merged

Simplified logging #532

merged 2 commits into from
Feb 10, 2018

Conversation

ejholmes
Copy link
Contributor

@ejholmes ejholmes commented Feb 8, 2018

Closes #527

This removes the "loop logger", and replaces it with a simple sequential logger. I think this solves all of the problems talked about in #527:

  1. This will work with parallelized execution.
  2. You get color output with --interactive.
  3. It removes the superfluous "pending" status.
  4. It scales easily with large stack configs.

Here's an example of what the output looks like:

asciicast

@codecov-io
Copy link

codecov-io commented Feb 8, 2018

Codecov Report

Merging #532 into master will decrease coverage by 0.65%.
The diff coverage is 46.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
- Coverage    88.6%   87.94%   -0.66%     
==========================================
  Files          94       91       -3     
  Lines        5987     5875     -112     
==========================================
- Hits         5305     5167     -138     
- Misses        682      708      +26
Impacted Files Coverage Δ
stacker/commands/stacker/__init__.py 95.83% <ø> (ø) ⬆️
stacker/context.py 97.22% <ø> (-0.04%) ⬇️
stacker/logger/__init__.py 0% <0%> (-100%) ⬇️
stacker/actions/base.py 63.79% <100%> (-1.79%) ⬇️
stacker/plan.py 90.8% <100%> (-0.78%) ⬇️
stacker/commands/stacker/base.py 84.28% <75%> (-1.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 075b083...7dc3234. Read the comment docs.

@russellballestrini
Copy link
Member

Wow, this is refreshing, nice work!

@ejholmes ejholmes mentioned this pull request Feb 9, 2018
4 tasks
log_level = logging.INFO
log_format = INFO_FORMAT
if sys.stdout.isatty():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this, thanks.

@phobologic
Copy link
Member

Ok, pushed a small update. @ejholmes can you take a look at why the test_util.py tests are all super noisy in tests now? See: https://circleci.com/gh/remind101/stacker/1404?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

(I thought it was something I had done, but it looks like it's been happening since this update)

@ejholmes ejholmes force-pushed the simple-logging branch 2 times, most recently from 6ee3ae4 to 1727ce0 Compare February 10, 2018 01:17
@ejholmes
Copy link
Contributor Author

Tests were noisy because the root logger in tests was being switched out with a StreamLogger in stacker/tests/test_stacker.py.

I added a commit to this that dependency injects the setup_logging function from scripts/stacker so unit tests can't fuck with logging.

@ejholmes ejholmes merged commit b9242f6 into master Feb 10, 2018
@ejholmes ejholmes added this to the 1.2 milestone Mar 1, 2018
troyready added a commit to troyready/stacker that referenced this pull request Mar 22, 2018
ejholmes pushed a commit that referenced this pull request Mar 23, 2018
ejholmes pushed a commit that referenced this pull request Mar 23, 2018
aarongorka pushed a commit to amaysim-au/stacker that referenced this pull request May 4, 2018
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants