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

Add logging levels #179

Merged
merged 20 commits into from
Sep 23, 2019
Merged

Add logging levels #179

merged 20 commits into from
Sep 23, 2019

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Sep 8, 2019

@sclevine @ekcasey @ameyer-pivotal @jromero before i go any further, can you confirm that this is the along the lines of what we discussed?

@sclevine
Copy link
Member

sclevine commented Sep 9, 2019

Looking great to me 😄

@@ -0,0 +1,134 @@
// Package logging implements the logger for the pack CLI.
Copy link
Member

Choose a reason for hiding this comment

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

Is the intent that pack would move to start using this implementation by having lifecycle as a dependency so that there is no duplication (and maintenance overhead)?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk. maybe we split the logging stuff out as its only lib?

Copy link
Member

Choose a reason for hiding this comment

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

@ekcasey what are your thoughts here?

Copy link
Contributor

@ameyer-pivotal ameyer-pivotal Sep 16, 2019

Choose a reason for hiding this comment

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

I considered the same thing way back when we began talking about a similar logger in lifecycle as we were using in pack. I think a separate repo might have some overhead with maintenance, but not sure if I like pack having to continue to depend on lifecycle...

Copy link
Member

Choose a reason for hiding this comment

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

I think we could consider this issue resolved. It seems like we are aligned on what the future would look like but not part of this issue.

@jromero jromero requested a review from ekcasey September 10, 2019 14:49
@jkutner
Copy link
Member Author

jkutner commented Sep 16, 2019

@sclevine @ekcasey Can you help figure out what I need to do to move this forward?

  • Should I break from the pattern pack uses for NewLogWithWriters?
  • Do we need to split out a logging lib, or wait?
  • Do we need to add log levels to all binaries, or can we merge just these first? (I don't actually think /lifecycle/builder will work the same way because we might want to preserve stdout/stderr from the buildpack, so I'd like to wait).

Copy link
Contributor

@ameyer-pivotal ameyer-pivotal left a comment

Choose a reason for hiding this comment

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

Nice work!

@jkutner jkutner changed the title [WIP] Add logging levels Add logging levels Sep 18, 2019
@jkutner
Copy link
Member Author

jkutner commented Sep 20, 2019

This is what it looks like in practice:

$ pack build myimage --builder heroku/buildpacks:18 --no-pull
Selected run image heroku/pack:18
Executing lifecycle version 0.0.0
Using build cache volume pack-cache-67f7abf58243.build
===> DETECTING
[detector] Success! (1)
===> RESTORING
[restorer] Restoring cached layer 'heroku/java:jdk'
[restorer] Restoring cached layer 'heroku/java:maven_m2'
===> ANALYZING
[analyzer] Analyzing image '3f066b64904aecb1541ad7468cf13efe913e068af7a1373a72bd4c352e926782'
[analyzer] Using cached layer 'heroku/java:jdk'
[analyzer] Using cached layer 'heroku/java:maven_m2'
[analyzer] Writing metadata for uncached layer 'heroku/java:jre'
===> BUILDING
[builder]
...
[builder]
===> EXPORTING
[exporter] Reusing layers from image with id '3f066b64904aecb1541ad7468cf13efe913e068af7a1373a72bd4c352e926782'
[exporter] *** Images:
[exporter]       index.docker.io/library/myimage:latest - succeeded
[exporter]
[exporter] *** Image ID: e6c614dd4f6d9da837391533c647fc562fbeffe2c29f8ae5800fd84f804dcec6
===> CACHING
[cacher] Reusing layer 'heroku/java:jdk'
[cacher] Caching layer 'heroku/java:maven_m2'
Successfully built image myimage

@jkutner jkutner merged commit 03bc9ca into master Sep 23, 2019
@jkutner jkutner deleted the log-levels branch September 23, 2019 18:55
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