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

moved aws-rollout script to logger instead of printing #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

akhon
Copy link

@akhon akhon commented Apr 14, 2022

please take a look. this is more powerful logging tool rather than plain printing

@AndrewFarley
Copy link
Collaborator

@akhon Hey thanks for the pull request.

Question: Do you think it would make more sense to actually use the various log levels in here? I believe I see you only used logger.info(). This script's verbose nature I felt was necessary so perhaps we keep it all at .info. However, I could argue if everything's .info then there's no purpose to have it using logging instead of print, no? You're not really adding value by changing the logger without using any features of it.

This may be an opportunity to make this script less verbose by default, and have only the critical pieces of info logged (under .info) and move some things over into .warn and/or into .debug or .error, etc. That could make the usage of this in CI/CD systems a bit less (overly) verbose. Would you like to take an attempt at doing this? I'd be happy to review and try your branch for one of my customers as well and see how I like it.

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.

2 participants