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 of Bindings Information for better UX #81

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

pivotal-david-osullivan
Copy link
Contributor

@pivotal-david-osullivan pivotal-david-osullivan commented Aug 31, 2021

Summary

Setting up bindings can be tricky - this adds logging when bindings are resolved, showing the binding name and keys so that users can see if they have been identified as expected during detection.

Use Cases

Users attempting to leverage bindings at build/run time

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).

@pivotal-david-osullivan pivotal-david-osullivan requested a review from a team August 31, 2021 21:20
@pivotal-david-osullivan pivotal-david-osullivan added semver:patch A change requiring a patch version bump type:enhancement A general enhancement labels Aug 31, 2021
@dmikusa
Copy link
Contributor

dmikusa commented Sep 2, 2021

I think we might need to rethink this a bit. Right not it's set such that a call to bindings.Resolve(..) will trigger this output.

  1. My first thought is that it seems conceivable that you could call Resolve multiple times and, I haven't tested it, but it seems like this patch would result in the logging output print multiple times. This is one of the things I don't like about how ConfigResolve logs the env variables. It happens when the resolver is created, which is an odd side-effect.

  2. I picked two buildpacks to test this with and neither seemed to work. Not the fault of this patch, but things I think we'll need to consider here.

    • I tried dependency-mapping first. This didn't work because it appears that the libpak code does not actually call bindings.Resolve(..). It basically implements its own Resolve. That's probably not good, so we could potentially look at making it use resolve as a solution.

    • I then tried using it with ca-certificates, but that didn't work because it's loading the bindings during detect. Thus the output is suppressed. You have to run with the pack build -v flag to get the verbose output. That makes it much less helpful, as the output is not part of build and obvious.

Thoughts:

  • It might make sense to see if we can get this upstream into libcnb. You could do this when libcnb is loading the bindings from disk. That would also only happen at most one time. On the negative side, it could be pretty verbose to always log this for every buildpack. While I think that works for Paketo buildpacks, other authors may not want that (maybe they don't use/support bindings?).

  • If it's not accepted upstream, we could perhaps do it at this point? I'm not 100% sure I like it here, but it would be around the same time, when build is started which seems like the right time (do it before stuff starts happening so the logging is at the top).

  • Another thought is that perhaps we can also fix my complaint with ConfigResolver and another minor complaint, which is that most build.go files for libpak buildpacks start out by creating a number of Resolvers (dependencies, build plan, config, etc..). It's a bunch of cut&paste when you start a new buildpack. It might be nice to have a helper method or object which could create these resolves, ensure a consistent point at which they'd only log once and reduce some boilerplate code in the buildpacks. I think we could do this in a non-breaking way to the API as well, since we'd be adding a method or struct to do this. Then again, this might be too ambitious.

Your thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants