Skip to content

Add a proper logger to fetch #90

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Feb 5, 2021
Merged

Add a proper logger to fetch #90

merged 14 commits into from
Feb 5, 2021

Conversation

robmorgan
Copy link
Contributor

@robmorgan robmorgan commented Feb 3, 2021 •
edited
Loading

This PR introduces a proper logger to fetch to ensure it's output is in line with other Gruntwork tools. It also makes integration testing much easier and solves the following issues:

Note: This PR must be merged after #88 as it improves upon it.

Design Notes

  • I noticed a lot of methods required multiple string params instead of a single FetchOptions struct, so I’m directly passing in a logger to follow the same pattern in most methods.
  • I added the logrus library but only saw the macOS binary increase by 100kb from 8.9M to ~9.0M.

Progress

  • Replaced all of the fmt.Printf calls.
  • Introduced the logrus library.

brikis98
brikis98 reviewed Feb 3, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Nice, thanks Rob!

We should probably add a --log-level param so you can control the amount of logging.

@robmorgan
Copy link
Contributor Author

Nice, thanks Rob!

We should probably add a --log-level param so you can control the amount of logging.

Thanks! Yeah, I intentionally avoided that and instead defaulted to 'INFO'. I wasn't sure if it made sense because I would either suppress a lot of the existing messages by default or we'd need to sprinkle in a few debug messages around the place to actually make it useful. I thought it would be easier to leave it out to under complicate this PR, but I agree it's something we'd want in the future. I could add it but default it to 'INFO' then people can override it to 'ERROR' or 'DEBUG'. What do you think?

@robmorgan
Copy link
Contributor Author

@brikis98 okay I've added a --log-level option and ported the logging code over to gruntwork-cli. To actually make the --log-level option worthwhile I've sprinkled in a few DEBUG log entries, so execute fetch using --log-level debug to see them.

@robmorgan robmorgan requested a review from brikis98 February 4, 2021 05:11
brikis98
brikis98 approved these changes Feb 4, 2021
View reviewed changes
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Very nice, thanks Rob! 👍

@robmorgan robmorgan merged commit 8a22319 into master Feb 5, 2021
@robmorgan robmorgan deleted the logger branch February 5, 2021 03:46
@robmorgan robmorgan mentioned this pull request Feb 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brikis98 brikis98 brikis98 approved these changes

@yorinasub17 yorinasub17 Awaiting requested review from yorinasub17

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robmorgan @brikis98