Removes the `Passed` field from `DetectResult`

[ForestEckhardt]

This boolean is an unneeded structure that can be accomplished with the addition of libcnb.Fail which is a way for a buildpack to create an error that the error handler will interpret as a detection failure as opposed to a detection error. This function also allows for the addition of messages that can clue the user in on why detection failed.

The new workflow would look as follows:

package main

import (
    "os"
    "path/filepath"

    "github.com/buildpacks/libcnb"
)

func Detect() libcnb.DetectFunc {
    return func(context libcnb.DetectContext) (libcnb.DetectResult, error) {
        ok := detectionCriteriaMet()
        if !ok {
            return libcnb.DetectResult{}, libcnb.Fail.WithMessage("application does not meet the detection criteria for this buildpack because of x reason")
        }

        return libcnb.DetectResult{}, nil
    }
}

This makes detection failure a distinctly different error than a normal error and allows the buildpack author to embed the decision making process directly into the failure message.

Signed-off-by: Forest Eckhardt feckhardt@pivotal.io

[dmikusa]

According to the spec, there are three states in which detect can finish:

• Pass (0)
• fail (100)
• error (1-99, 101+)

Right now, the results of detect allow you to select between them.

• If Pass = true, then this results in exit Pass (0).
• If Pass = false, then this results in fail (100).
• If detect returns an error that results in error (1) and the error is logged.

IMHO, that maps pretty cleanly to what the spec is doing. I don't think it's any more verbose than what's being proposed either.

• Pass - return libcnb.DetectResult{Pass: true}, nil
• Fail - return libcnb.DetectResult{}, nil (false is implicit, but you could set it if you wanted to be explicit)
• Error - return libcnb.DetectResult{}, fmt.Errorf("shoelaces were untied")

I'm not sure I follow how the change you're proposing would work for these scenarios or how it would improve the API. Can you give examples and expand on the benefits to what you're proposing?

[ForestEckhardt]

@dmikusa-pivotal Let me kick off with saying that this is the technique that is being used in packit so there may be a bit of homerism with this request.

I think that what I am trying to accomplish is to distinguish between an error and a detection failure while still allowing the buildpack to reveal to the user why the detection was not satisfied by leveraging the existing error structures that are baked into Go. In the current structure there is no way to express to a user why they failed without attaching an error and then you get an error not a failure.

By having me (the buildpack author) return a concrete error type I am being very intentional about when I fail the buildpack due to not meeting detection criteria and when I am intentional I should express to the user the decision that I made. This interface forces me to be intentional and gives me an avenue to express my intention all in fairly idiomatic go terms.

Let me see if something like this example helps which will be a semi real world expansion of my workflow example:

package main

import (
    "errors"
    "os"
    "path/filepath"

    "github.com/buildpacks/libcnb"
)

func Detect() libcnb.DetectFunc {
    return func(context libcnb.DetectContext) (libcnb.DetectResult, error) {
        _, err := os.Stat(filepath.Join(context.Application.Path, "package.json"))
        if err != nil {
            if errors.Is(err, os.ErrNotExist) {
                return libcnb.DetectResult{}, libcnb.Fail.WithMessage("no package.json found in application directory")
            }
            return libcnb.DetectResult{}, err
        }
        return libcnb.DetectResult{}, nil
    }
}

In this example we can see all three paths that can be taken:

• Pass - If there is a package.json located in the application directory then the detection criteria has been met and we can pass.
• Fail - If there is a no package.json located in the application directory the detection criteria has not been met and we can return a libcnb.Fail with a message explaining why the detection criteria.
• Error - If there is a problem stating package.json in the application directory we return the resulting error and be a build failure.

Hopefully what this example demonstrates is that it allows buildpack authors to communicate more about the decisions that their buildpacks are making, which in turn makes the buildpacks themselves more translucent. I am not necessarily married to this exact implementation but I do think that we need to provide a way for authors to peel back the curtain and show users the inner decision making.

tl;dr

I think that we need some way of allowing buildpack authors to communicate the decision making process of detect and in particular what detection criteria was not met. I believe the simplest and most idiomatic way of doing this through a small custom error like the one purposed.

[dmikusa]

OK, thanks for the context. I think I understand now.

I think my objection to this would be with the intent of the API. You have pass, fail and error. It seems logical to attach an error for the error case, but I'm not sold it's the right thing to do for the fail case.

The spec defines Pass, Fail, and Error, but IMHO Fail isn't a great way to describe this case. First, it's too close in meaning to error that it's confusing. Second, what typically happens in the buildpacks that I maintain that go down this path is not a failure, but rather that a buildpack decides it doesn't want to run.

There isn't a problem. Nothing went wrong. Nothing failed. It just opted out. The buildpack doesn't have anything to contribute for a particular type of application, and that's OK. I'm not sure it's a complete match to your example but sticking with that, if there's no package.json then NPM just doesn't run. That's not necessarily an error, maybe I just don't need NPM to install anything.

Given that, it feels to me like the proposed API change puts too much emphasis on some problem happening because it associates that "fail" scenario (which isn't necessarily a failure, as explained above) with a go error, and that indicates a failure.

I think at the moment in the buildpacks I work on we're just using the logger to log that and then returning. That works well enough for what we're communicating. It also gives the option of controlling the level at which that get's logged. I think we typically put that stuff at DEBUG level because it's useful to have if something is not working right, but it's not something I want to see every time the buildpacks run. Take your package.json example, if every time the buildpacks runs it says no package.json exists, that would get old for an app where I am not using NPM to install anything.

If you feel like this is something we should do, I'm not opposed to the idea of being able to attach a message to the "fail" scenario. What about putting it on the DetectResult object?

You could have some convenient methods to make this easier, similar to yours.

• libcnb.Pass returns libcnb.DetectResult{Pass: true}, nil
• libcnb.Fail returns libcnb.DetectResult{}, nil
• libcnb.Fail.WithMessage("umbrella is required") returns libcnb.DetectResult{Pass: false, FailMessage: "umbrella is required"}, nil
• libcnb.Error.With(fmt.Errorf("something exploded")) returns libcnb.DetectResult{}, err

Then libcnb, if Pass is false and there's a fail message could log this out (like it does now for the error message). As I mentioned above, I'd prefer at Debug level for fail messages. Perhaps that could be configurable if there's debate?

[ryanmoran]

Ignoring the discussion about providing a more natural API to allow buildpack authors to provide context when failing detection, I want to express my thoughts on the current API and its alignment to the spec.

I don't really see the clean mapping of the current implementation onto the spec. The spec outlines that the passing, failing, or erroring of the detection phase maps onto a single output which is the exit code of the executable.

Stated plainly,

Pass = 0
Fail = 100
Error = 1-99, 101-255

Now, the current libcnb implementation allows for the buildpack author to return the following states:

Pass	Error	Intention
true	nil	detection has succeeded
true	err	???
false	nil	detection has failed
false	err	detection has errored

As you can see from the table, we have this weird state where the detect result is marked as passing, but an error is also returned. This is obviously cleared up by the special knowledge that the return values of this function have precedence. The error value is more important than the passing result and thus will indicate that the buildpack author intends to indicate that detection has failed.

This is not to mention that we have also split the indication of the detect executable exit code into two different outputs (list ordering indicates precedence).

Field	Result
error	exit 1-99, 101-255
libcnb.DetectResult.Pass = true	exit 0
libcnb.DetectResult.Pass = false	exit 100

Instead, if we think about the error return value as representing the exit code of the executable, then the mapping is a bit more direct:	Field	Result
error = nil	exit 0
error = libcnb.Fail	exit 100
error = errors.New('something bad happened')	exit 1-99, 101-255

Since the single field maps to the exit code and the choices for the single field are mutually exclusive, there isn't any special knowledge outlining an order of precedence, and it isn't possible to express a return value that is nonsensical according to the specifcation, like Pass = true and error = errors.New("my error").

Now, this may really boil down to a matter of opinion, and in these cases, I don't really expect to win an argument if the current implementation already has the incumbent advantage. Its just worth considering that what is described here as intuitive may only seem that way because of repeated use.

[ekcasey]

I tend to agree with @dmikusa-pivotal. "Fail", while being the technically correct term per the CNB spec, is a bit misleading and could be more verbosely described as "successfully decided not to participate". In general I think it is best to use go errors for unexpected/bad things, not for control flow in a common/expected situations.

@ryanmoran,

As you can see from the table, we have this weird state where the detect result is marked as passing, but an error is also returned. This is obviously cleared up by the special knowledge that the return values of this function have precedence. The error value is more important than the passing result and thus will indicate that the buildpack author intends to indicate that detection has failed.

I don't see this situation as being any more special than any other go function that returns a result and an error. Go has a strong convention that if the returned error is not nil, it should be handled first, and other return values should be treated as unreliable.

Conventionally most folks will return empty version of other results when returning a non-empty error. So, buildpacks should be returning libcnb.DetectResult{}, err in the case of an error. But, if for whatever reason they return libcnb.DetectResult{Pass: true}, err it is clear by typical go conventions, that the error should be handled and the result should be ignored.

Taking for example:

 resp, err := http.Get("http://example.com/")

A successfully executed GET request where the server returned a 500 would include that code in the resp while the error would be nil. If the request failed due to a timeout or something an error would be returned. If both an error and a response object with a status code were returned... while that would be weird and the stdlib doesn't do that, but it's clear what to do next, the client code should handle the error and ignore the result.

[samj1912]

Closing this per discussions we had in the Paketo WG meeting around this PR.