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

Adds Info and Debug logger functionality #254

Merged
merged 13 commits into from
Dec 2, 2021
Merged

Adds Info and Debug logger functionality #254

merged 13 commits into from
Dec 2, 2021

Conversation

ForestEckhardt
Copy link
Contributor

Resolves #252

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@fg-j
Copy link

fg-j commented Nov 18, 2021

This is awesome. Could you add some godoc examples of how a buildpack author can incorporate this logger into their buildpack in practice? The tests sort of show it, but a direct example would be great.

@ForestEckhardt
Copy link
Contributor Author

For sure I could doc up the whole package if that is something we are looking for!

@ryanmoran
Copy link
Member

There's quite a bit of duplication happening here. We've now got 3 different loggers with duplicative implementation APIs and switching logic. I think we can achieve something a bit simpler by just controlling whether or not the writer goes somewhere or is discarded. I've thrown together a simplified example here: https://github.com/paketo-buildpacks/packit/blob/log-levels-alt/scribe/logger.go

Take a look and let me know what you think.

@ForestEckhardt
Copy link
Contributor Author

I like the look of that implementation. I am gonna make those changes to my code as well as add documentation!

@ForestEckhardt ForestEckhardt marked this pull request as draft November 18, 2021 21:50
@ForestEckhardt
Copy link
Contributor Author

ForestEckhardt commented Nov 18, 2021

Moving this to draft until I have the examples written up as well as a package level doc!

@ForestEckhardt ForestEckhardt marked this pull request as ready for review November 29, 2021 16:04
@ForestEckhardt
Copy link
Contributor Author

@fg-j @ryanmoran Alrighty took me a little longer than I thought but the docs are up and running. Couple call outs.

  • Ryan and I have decided to deprecate the Bar object as it is not fully implemented and we do not foresee it being used in the near future and and we are going to use it we would like to overhaul the interface.
  • I could not get a tested example working for the Color object because of that I also do not put together examples for the writer. I could make an indented writer but it did not seem completely necessary to me to set up examples. If you would like an example just let me know.
  • I just put together the best blurb that I could for the top level description I am more than happy to get feedback on this one.

Hope this accomplishes everything that y'all are looking for.

Copy link

@fg-j fg-j left a comment

Choose a reason for hiding this comment

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

Maybe I didn't understand your comment @ForestEckhardt about getting a working example with Color, but if possible I'd love to see an example of how to properly use the logger with Debug functionality.

scribe/logger.go Outdated Show resolved Hide resolved
scribe/logger.go Outdated Show resolved Hide resolved
scribe/logger.go Outdated Show resolved Hide resolved
scribe/logger.go Outdated Show resolved Hide resolved
scribe/logger.go Outdated Show resolved Hide resolved
scribe/writer.go Outdated Show resolved Hide resolved
scribe/writer.go Outdated Show resolved Hide resolved
scribe/writer.go Outdated Show resolved Hide resolved
scribe/writer.go Outdated Show resolved Hide resolved
scribe/writer.go Outdated Show resolved Hide resolved
ForestEckhardt and others added 2 commits November 29, 2021 11:36
Co-authored-by: Frankie G-J <frankieg@vmware.com>
Co-authored-by: Frankie G-J <frankieg@vmware.com>
@ForestEckhardt
Copy link
Contributor Author

Maybe I didn't understand your comment @ForestEckhardt about getting a working example with Color, but if possible I'd love to see an example of how to properly use the logger with Debug functionality.

My trouble with Color is that the only way I know haw to test the examples is by testing the contents of stdout. The Colors function adds a prefix and a suffix that is interpreted by the terminal and changes the display but seemingly does not make it to standard out. If there is another way of getting the examples working and testing it I am game!

I totally forgot that was the feature I added to start this work 🤦 I will add an example ASAP!

@ForestEckhardt
Copy link
Contributor Author

@fg-j So after so discussion with @ryanmoran and a little investigating on his part he discovered that:

The terminal escape sequences are dropped if the output is not a TTY, and the io.Pipe that the Examples feature uses is not a TTY.

So it would appear that it is not possible to test the Color functionality using the Go Examples testing framework. I hope that clears things up.

@ForestEckhardt ForestEckhardt requested a review from fg-j November 30, 2021 16:48
@ForestEckhardt ForestEckhardt added the semver:minor A change requiring a minor version bump label Dec 1, 2021
@ryanmoran ryanmoran added this to the v2.0.0 milestone Dec 2, 2021
fg-j
fg-j previously approved these changes Dec 2, 2021
Copy link

@fg-j fg-j left a comment

Choose a reason for hiding this comment

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

Thanks for adding that debug logging example. LGTM!

@fg-j fg-j requested review from ryanmoran and removed request for ryanmoran December 2, 2021 18:48
scribe/logger.go Outdated Show resolved Hide resolved
Co-authored-by: Ryan Moran <155736+ryanmoran@users.noreply.github.com>
@paketo-bot paketo-bot removed the semver:minor A change requiring a minor version bump label Dec 2, 2021
@ForestEckhardt ForestEckhardt added the semver:minor A change requiring a minor version bump label Dec 2, 2021
ryanmoran
ryanmoran previously approved these changes Dec 2, 2021
@paketo-bot paketo-bot removed the semver:minor A change requiring a minor version bump label Dec 2, 2021
@ForestEckhardt ForestEckhardt added the semver:minor A change requiring a minor version bump label Dec 2, 2021
@fg-j fg-j self-requested a review December 2, 2021 20:25
@ForestEckhardt ForestEckhardt merged commit 27ca62d into main Dec 2, 2021
@ForestEckhardt ForestEckhardt deleted the log-levels branch December 2, 2021 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for BP_LOG_LEVEL in scribe
4 participants