Skip to content
This repository has been archived by the owner on Mar 13, 2021. It is now read-only.

Spike: Invoker buildpacks #51

Merged
merged 9 commits into from
Feb 27, 2019
Merged

Conversation

scothis
Copy link
Member

@scothis scothis commented Feb 14, 2019

detect/main.go Outdated
detected = append(detected, "java")
for name := range detect.BuildPlan {
if strings.HasPrefix(name, "riff-invoker-") {
detected = append(detected, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we strip the prefix here?

}

if code, err := bc.detect(detect); err != nil {
detect.Logger.Info(err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use something else than Info (or at least use visual cue that we're dealing with an error)

Copy link
Member Author

Choose a reason for hiding this comment

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

The logger only exposes Debug and Info levels. It appears the newer version of libcfbuildpack support more logging levels.

build, err := build.DefaultBuild()
if err != nil {
_, _ = fmt.Fprintf(os.Stderr, "Failed to initialize Build: %s\n", err)
os.Exit(101)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
os.Exit(101)
os.Exit(Error_Initialize)

}

build.Logger.Info("Buildpack passed detection but did not know how to actually build. Should never happen.")
return build.Failure(104), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return build.Failure(104), nil
return build.Failure(Error_DetectInternalError), nil

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a var named Error_BuildInternalError since this is not part of the detect phase.

build.Logger.FirstLine(build.Logger.PrettyIdentity(build.Buildpack))

if invoker, ok, err := bc.buildpack.Invoker(build); err != nil {
return build.Failure(105), err
Copy link
Contributor

Choose a reason for hiding this comment

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

error code should be internal error I think

return build.Failure(105), err
} else if ok {
if err = invoker.Contribute(); err != nil {
return build.Failure(106), err
Copy link
Contributor

Choose a reason for hiding this comment

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

idem, internal error here

)

type Buildpack interface {
Name() string
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename (or add) to Key or something, as this is something that is tested for strict equality with == and is not meant to be pretty displayable (like a product name)

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to Id()

@scothis scothis merged commit a7d17e6 into projectriff:master Feb 27, 2019
@scothis scothis deleted the invoker-buildpacks branch February 27, 2019 19:21
@nebhale nebhale added semver:patch A change requiring a patch version bump type:enhancement A general enhancement labels Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

3 participants