Extract method for generating Config

[dmikusa]

This PR extracts the code to create a Config object from a list of applied Options. The method also allows us to enforce a global set of default options.

I'm submitting this change for two reasons:

1. There was code duplicated across main/build/detect to generate a Config object. This change refactors that duplicate code out.
2. As a buildpack implementer, you will write your own BuildFunc/DetectFunc methods. These take a DetectContext/BuildContext as the single parameter. A buildpack author is going to want a logger. It may be that the author creates their own, but we are already asking the author to supply a libcnb.Logger implementation in their main build/detect method. This would essentially ask them to create two separate loggers. Instead, libcnb has the one provided to the main build/detect, we can just pass this dependency along though the DetectContext/BuildContext.

Signed-off-by: Daniel Mikusa dmikusa@vmware.com

[ForestEckhardt]

2. As a buildpack implementer, you will write your own BuildFunc/DetectFunc methods. These take a DetectContext/BuildContext and a list of Options. It would be tedious for buildpack implementors to need to create their own Config object to get access to things like the Logger, TOML Writer, Exit Handler, etc.. (everything on Config).

I am generally in favor of this change, seems like a cogent refactor. I am however confused by this wording. Is the construction of the config struct not complete abstracted away from the buildpack author as any interface they have to influence the options is through a variadic function signature with the object type being an Option not a Config? I am confused where the buildpack author interacts with the Config type directly.

[dmikusa]

2. As a buildpack implementer, you will write your own BuildFunc/DetectFunc methods. These take a DetectContext/BuildContext and a list of Options. It would be tedious for buildpack implementors to need to create their own Config object to get access to things like the Logger, TOML Writer, Exit Handler, etc.. (everything on Config).

I am generally in favor of this change, seems like a cogent refactor. I am however confused by this wording. Is the construction of the config struct not complete abstracted away from the buildpack author as any interface they have to influence the options is through a variadic function signature with the object type being an Option not a Config? I am confused where the buildpack author interacts with the Config type directly.

It is now with this PR. It was not before.

Without this PR, your DetectFunc/BuildFunc gets a list of Options. If you want the value from one of those, you have to run the Option funcs and build your own Config object. With this PR, you just call https://github.com/buildpacks/libcnb/pull/151/files#diff-0e426a43248661127a0c0ee115aef7a1093b635f8993b3f7ebb1dd9f05b8f249R100 and pass in the Options list you were given.

As I mentioned on the note above, we could remove the need to call that function entirely, but you'd then have to pass the Config object around, which isn't as convenient as the variadic Options list. Definitely interested in thoughts on the approach we take here.

[ForestEckhardt]

Maybe I am confusing some terminology here, either that or i am having a fundamental misunderstanding with what you are trying to accomplish. When you say buildpack implementors are you talking about the authors of buildpacks that are using libcnb or are you talking about maintainers of libcnb? If you are talking about the latter then your initial comment and follow up make a lot more sense. If you are referring to the former that is where my confusion kicks in because I do not see how this changes has any impact on an author that is using this library to build a buildpack.

[dmikusa]

Hmm, I think I was getting something confused yesterday. Staring at this code too long. Let me put this into draft and rethink. We definitely need a change here, but I want to try a couple of options. BRB.

[dmikusa]

OK, I updated the code & posted a new description here. If you all could take a look. Much appreciated!

[ryanmoran]

@dmikusa-pivotal Can you provide an example of what kind of code this is cleaning up?

Since I can't really think of how I would use this, I'm having trouble valuing its positive impact against the clearer negative that now every Build signature in every buildpack we maintain would need to be modified to include a reference to a variable we wouldn't likely use.

[dmikusa]

Sure.

1. So the main entry point looks like this:

func main() {
    libcnb.Main(profile.Detect, profile.Build)
}

You can optionally provide additional arguments, like a logger, or you can take the defaults. The defaults will include a logger. The plan for libpak/Paketo buildpacks is to have a custom log.Logger implementation that we initialize and pass through to libcnb, so that output is all consistently formatted.

2. The libcnb.Main method calls the passed in DetectFunc/BuildFunc as appropriate. In the present state, the logger used by libcnb.Main is unavailable.

As a buildpack author, my DetectFunc implementation looks like this, I want to log things in the implementation.

func Detect(context libcnb.DetectContext) (libcnb.DetectResult, error) {
        log.Info("hi")  // fails

    // TODO: implement detection logic
    return libcnb.DetectResult{Pass: false}, nil
}

Because we changed Detect & Build from an Interface (we used to implement that on a struct that is used to carry the logger to functions), the logger's not present anymore. The author has to create a logger which is painful during testing. We need to pass in the logger to validate the output, it's not the best way to test things but sometimes there isn't another way to validate a certain condition has occurred.

At any rate, I'm not 100% tied to this implementation. I think we could achieve the same thing by attaching the logger to the DetectContext/BuildContext that is already passed into the method. If you would prefer that, I'd be OK going that route.

I'm open to other suggestions as well. How is packit handling this type of thing?

[ryanmoran]

Wouldn't your DetectFunc implementation look more like this?

func Detect(logger libcnb.Logger) libcnb.DetectFunc {
  return func(context libcnb.DetectContext) (libcnb.DetectResult, error) {
    log.Info("hi")  // fails

    // TODO: implement detection logic
    return libcnb.DetectResult{Pass: false}, nil
  }
}

[dmikusa]

TBH, I haven't really done much with the consumer side yet. We just started working on the profile buildpack which is the first one to directly consume libcnb v2. My initial approach was to keep it as simple as possible and try to avoid the need for returning anonymous functions. That was an annoyance with the contributors approach of libcnb v1 (IMHO).

I don't think that would help in the case of consuming the default logger either.

func main() {
        // now I have to create a logger here, but it's still different from what get's generated by default
    libcnb.Main(profile.Detect(log), profile.Build(log))
}

[dmikusa]

If everyone could take a look. I've refactored this PR so that it no longer changes the DetectFunc/BuildFunc method signatures. It now passes the logger through the DetectContext/BuildContext.