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.

Sign up for GitHub

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

Returns error if build does not start successfully with kp create image --wait #1309

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Returns error if build does not start successfully with kp create image --wait #1309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions pkg/logs/wait_for_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ import (
"fmt"
"io"

"github.com/pivotal/kpack/pkg/apis/build/v1alpha1"
corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1"
"github.com/pivotal/kpack/pkg/client/clientset/versioned"
"github.com/pkg/errors"

v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/tools/cache"
watchTools "k8s.io/client-go/tools/watch"

"github.com/pivotal/kpack/pkg/apis/build/v1alpha1"
corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1"
"github.com/pivotal/kpack/pkg/client/clientset/versioned"
)

type imageWaiter struct {
Expand Down Expand Up @@ -97,17 +97,22 @@ func imageFailure(name, statusMessage string) error {
}

func (w *imageWaiter) waitBuild(ctx context.Context, writer io.Writer, namespace, buildName string) (string, error) {
doneChan := make(chan struct{})
defer func() { <-doneChan }()

go func() { // tail logs
defer close(doneChan)
err := w.logTailer.TailBuildName(ctx, writer, namespace, buildName)
if err != nil {
fmt.Fprintf(writer, "error tailing logs %s", err)
}
}()

err := w.imageBuildStarted(ctx, namespace, buildName)
if err != nil {
fmt.Fprintf(writer, "Build failed to start: %s", err)
return "", nil
Comment on lines +102 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

We should toss the error back up the chain instead of trying to handle it here

Suggested change
fmt.Fprintf(writer, "Build failed to start: %s", err)
return "", nil
return "", fmt.Errorf("build failed to start: %s", err)

} else {
doneChan := make(chan struct{})
defer func() { <-doneChan }()

go func() { // tail logs
defer close(doneChan)
err := w.logTailer.TailBuildName(ctx, writer, namespace, buildName)
if err != nil {
fmt.Fprintf(writer, "error tailing logs %s", err)
}
}()
}
Comment on lines +101 to +115
Copy link
Contributor

Choose a reason for hiding this comment

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

if a {
  ...
  return
} else {
  bar()
}

is better written as

if a {
  ...
  return
}
bar()

build, err := w.buildWatchUntil(ctx, namespace, buildName, filterErrors(buildHasResolved))
if err != nil {
return "", err
Expand Down Expand Up @@ -173,3 +178,16 @@ func filterErrors(condition watchTools.ConditionFunc) watchTools.ConditionFunc {
return condition(event)
}
}

func (w *imageWaiter) imageBuildStarted(ctx context.Context, namespace, buildName string) error {
build, err := w.KpackClient.KpackV1alpha1().Builds(namespace).Get(ctx, buildName, v1.GetOptions{})
if err != nil {
return err
}

condition := build.Status.GetCondition(corev1alpha1.ConditionSucceeded)
if condition.IsFalse() {
return errors.New(condition.Message)
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 errors.New(condition.Message)
return buildFailure(condition.Message)

kpack/pkg/logs/wait_for_image.go

Lines 137 to 144 in ca60665

func buildFailure(statusMessage string) error {
errMsg := "build failed"
if statusMessage != "" {
errMsg = fmt.Sprintf("%s: %s", errMsg, statusMessage)
}
return errors.New(errMsg)
}

}
return nil
}
8 changes: 6 additions & 2 deletions pkg/logs/wait_for_image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"io"
"testing"

"k8s.io/apimachinery/pkg/runtime"

"github.com/sclevine/spec"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -96,9 +98,11 @@ func testWaitForImage(t *testing.T, when spec.G, it spec.S) {
_, err := clientset.KpackV1alpha1().Images(namespace).Create(context.TODO(), image, metav1.CreateOptions{})
require.NoError(t, err)

_, err = clientset.KpackV1alpha1().Builds(namespace).Create(context.TODO(), successfulBuild, metav1.CreateOptions{})
build, err := clientset.KpackV1alpha1().Builds(namespace).Create(context.TODO(), successfulBuild, metav1.CreateOptions{})
require.NoError(t, err)

clientset.PrependReactor("get", "builds", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
return true, build, nil
})
result, err := imageWaiter.Wait(context.TODO(), out, image)
assert.NoError(t, err)
assert.Equal(t, successfulBuild.Status.LatestImage, result)
Expand Down