Start buildrun follow logs test #39

gabemontero · 2021-09-20T21:56:17Z

Changes

add unit test for previously provided live log follow / tail of invoking build run from referenced build via cli

unit tests uncovered some race conditions previously unseen with manual testing

/kind cleanup

/assign @otaviof
/assign @coreydaley
/assign @HeavyWombat

after a bunch of back and forth with myself today (preceded by a conversation with @otaviof )
I gave up trying to get k8s fake clients to work with watches, etc. and am just testing the function in this package

we have tests in pod_watcher and tail for those functions

in looking at the panic we saw in @HeavyWombat 's #35 I do believe line 64 in run.go is in the Run() method, so in theory this test calling that method would have caught that error.

But as @otaviof mentioned to me today, and I agree, e2e tests that run against an api server are also needed for this component long term

Submitter Checklist

[ /] Includes tests if functionality changed/was added
[ n/a] Includes docs if changes are user-facing
[ /] Set a kind label on this PR
[/ ] Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

NONE

gabemontero · 2021-09-20T22:01:10Z

I was able to test successfully manuall as well

gmontero ~/go/src/github.com/shipwright-io/cli-1  (start-buildrun-follow-logs-test)$ shp build run buildpack-nodejs-build --follow
Pod 'buildpack-nodejs-build-8gftk-72fnp-pod-h878h' is in state "Pending"...
Pod 'buildpack-nodejs-build-8gftk-72fnp-pod-h878h' is in state "Pending"...
Pod 'buildpack-nodejs-build-8gftk-72fnp-pod-h878h' is in state "Pending"...
[place-tools] 2021/09/20 21:58:48 Copied /ko-app/entrypoint to /tekton/tools/entrypoint
[source-default] 2021/09/20 21:58:56 warning: unsuccessful cred copy: ".docker" from "/tekton/creds" to "/": unable to create destination directory: mkdir /.docker: permission denied
[source-default] {"level":"info","ts":1632175177.0604281,"logger":"git","msg":"ssh","path":"/usr/bin/ssh","version":"OpenSSH_8.0p1, OpenSSL 1.1.1g FIPS  21 Apr 2020"}
[source-default] {"level":"info","ts":1632175177.0617015,"logger":"git","msg":"git","path":"/usr/bin/git","version":"git version 2.27.0"}
[build-and-push] ===> DETECTING
[build-and-push] 5 of 9 buildpacks participating
[build-and-push] paketo-buildpacks/ca-certificates 2.4.1
[build-and-push] paketo-buildpacks/node-engine     0.8.0
[build-and-push] paketo-buildpacks/npm-install     0.4.0
[build-and-push] paketo-buildpacks/node-module-bom 0.1.2
[build-and-push] paketo-buildpacks/npm-start       0.3.0
[build-and-push] ===> ANALYZING
[source-default] {"level":"info","ts":1632175177.0671594,"logger":"git","msg":"git-lfs","path":"/usr/bin/git-lfs","version":"git-lfs/2.11.0 (GitHub; linux amd64; go 1.14.4)"}
[source-default] {"level":"debug","ts":1632175177.067215,"logger":"git","msg":"/usr/bin/git clone --quiet --no-tags --depth 1 --single-branch -- https://github.com/shipwright-io/sample-nodejs /workspace/source"}
[source-default] {"level":"debug","ts":1632175177.9048932,"logger":"git","msg":"/usr/bin/git -C /workspace/source submodule update --init --recursive"}
[source-default] {"level":"debug","ts":1632175177.940991,"logger":"git","msg":"/usr/bin/git -C /workspace/source rev-parse --verify HEAD"}
[build-and-push] Restoring metadata for "paketo-buildpacks/ca-certificates:helper" from app image
[build-and-push] ===> RESTORING
[build-and-push] ===> BUILDING
[build-and-push] 
[build-and-push] Paketo CA Certificates Buildpack 2.4.1
[build-and-push]   https://github.com/paketo-buildpacks/ca-certificates
[build-and-push]   Launch Helper: Contributing to layer
[build-and-push]     Creating /layers/paketo-buildpacks_ca-certificates/helper/exec.d/ca-certificates-helper
[build-and-push] Paketo Node Engine Buildpack 0.8.0
[build-and-push]   Resolving Node Engine version
[build-and-push]     Candidate version sources (in priority order):
[build-and-push]       package.json -> ">= 0.10.12"
[build-and-push]       <unknown>    -> ""
[build-and-push] 
[build-and-push]     Selected Node Engine version (using package.json): 16.9.1
[build-and-push] 
[build-and-push]   Executing build process
[build-and-push]     Installing Node Engine 16.9.1
[build-and-push]       Completed in 3.035s
[build-and-push] 
[build-and-push]   Configuring build environment
[build-and-push]     NODE_ENV     -> "production"
[build-and-push]     NODE_HOME    -> "/layers/paketo-buildpacks_node-engine/node"
[build-and-push]     NODE_VERBOSE -> "false"
[build-and-push] 
[build-and-push]   Configuring launch environment
[build-and-push]     NODE_ENV     -> "production"
[build-and-push]     NODE_HOME    -> "/layers/paketo-buildpacks_node-engine/node"
[build-and-push]     NODE_VERBOSE -> "false"
[build-and-push] 
[build-and-push]     Writing profile.d/0_memory_available.sh
[build-and-push]       Calculates available memory based on container limits at launch time.
[build-and-push]       Made available in the MEMORY_AVAILABLE environment variable.
[build-and-push] 
[build-and-push] Paketo NPM Install Buildpack 0.4.0
[build-and-push]   Resolving installation process
[build-and-push]     Process inputs:
[build-and-push]       node_modules      -> "Not found"
[build-and-push]       npm-cache         -> "Not found"
[build-and-push]       package-lock.json -> "Not found"
[build-and-push] 
[build-and-push]     Selected NPM build process: 'npm install'
[build-and-push] 
[build-and-push]   Executing build process
[build-and-push]     Running 'npm install --unsafe-perm --cache /layers/paketo-buildpacks_npm-install/npm-cache'
[build-and-push]       Completed in 1.158s
[build-and-push] 
[build-and-push]   Configuring launch environment
[build-and-push]     NPM_CONFIG_LOGLEVEL -> "error"
[build-and-push] 
[build-and-push]   Configuring environment shared by build and launch
[build-and-push]     PATH -> "$PATH:/layers/paketo-buildpacks_npm-install/modules/node_modules/.bin"
[build-and-push] 
[build-and-push] 
[build-and-push] Paketo Node Module Bill of Materials Generator Buildpack 0.1.2
[build-and-push]   Resolving CycloneDX Node.js Module version
[build-and-push]     Selected CycloneDX Node.js Module version: 3.0.7
[build-and-push] 
[build-and-push]   Executing builbuildpack-nodejs-buildd process
[build-and-push]     Installing CycloneDX Node.js Module 3.0.7
[build-and-push]       Completed in 278ms
[build-and-push] 
[build-and-push]   Configuring environment
[build-and-push]     Appending CycloneDX Node.js Module onto PATH
[build-and-push] 
[build-and-push]   Running CycloneDX Node.js Module
[build-and-push]     Running 'cyclonedx-bom -o bom.json'
[build-and-push]       Completed in 268ms
[build-and-push] 
[build-and-push] Paketo NPM Start Buildpack 0.3.0
[build-and-push]   Assigning launch processes
[build-and-push]     web: node server.js
[build-and-push] 
[build-and-push] ===> EXPORTING
[build-and-push] Reusing layers from image 'index.docker.io/gmontero/sample-nodejs@sha256:726b5f3298c437305907ed3e3e0f758e9a355840dfa29e86d816bd7655d6196e'
[build-and-push] Adding layer 'paketo-buildpacks/ca-certificates:helper'
[build-and-push] Adding layer 'paketo-buildpacks/node-engine:node'
[build-and-push] Reusing layer 'paketo-buildpacks/npm-install:modules'
[build-and-push] Reusing 1/1 app layer(s)
[build-and-push] Reusing layer 'launcher'
[build-and-push] Adding layer 'config'
[build-and-push] Adding label 'io.buildpacks.lifecycle.metadata'
[build-and-push] Adding label 'io.buildpacks.build.metadata'
[build-and-push] Adding label 'io.buildpacks.project.metadata'
[build-and-push] Saving docker.io/gmontero/sample-nodejs:latest...
[build-and-push] *** Images (sha256:5bd7f92c9623c6d6a0d78801df3ed434d844824c67cb9a0dd6c87cb74ae49b5c):
[build-and-push]       docker.io/gmontero/sample-nodejs:latest
[build-and-push] Adding cache layer 'paketo-buildpacks/node-engine:node'
[build-and-push] Adding cache layer 'paketo-buildpacks/npm-install:modules'
[build-and-push] Adding cache layer 'paketo-buildpacks/node-module-bom:cyclonedx-node-module'
Pod 'buildpack-nodejs-build-8gftk-72fnp-pod-h878h' has succeeded!
gmontero ~/go/src/github.com/shipwright-io/cli-1  (start-buildrun-follow-logs-test)$

otaviof

It looks good, just a couple of minor comments.

otaviof · 2021-09-21T09:09:38Z

pkg/shp/cmd/build/run.go

@@ -141,6 +146,9 @@ func (r *RunCommand) stop() {

 // Run creates a BuildRun resource based on Build's name informed on arguments.
 func (r *RunCommand) Run(params *params.Params, ioStreams *genericclioptions.IOStreams) error {
+	// ran into some data race conditions during unit test with this starting up, but pod events


Nice 👍🏼 Good catch!

otaviof · 2021-09-21T09:10:46Z

pkg/shp/cmd/build/run_test.go

+		},
+	}
+	shpclientset := shpfake.NewSimpleClientset()
+	createReactorFunc := func(action fakekubetesting.Action) (handled bool, ret runtime.Object, err error) {


nit: would be worth to explain why we need this reactor in place.

good point / part of next push

otaviof · 2021-09-21T09:12:03Z

pkg/shp/cmd/build/run_test.go

+	pod.Status.Phase = corev1.PodSucceeded
+	cmd.onEvent(pod)
+	if !strings.Contains(out.String(), "Pod 'testpod' has succeeded!") {
+		t.Fatalf("unexpected output: %s", out.String())


nit: alternatively we could use gomega assertion library, as we do in the other unit-tests.

yeah I've see that and am not the biggest fan ... was hoping for some developer choice here :-)

but let me ponder and then "officially" respond

I would like to see gomega kept out of unit tests, especially just for an assertion.

otaviof · 2021-09-21T09:12:25Z

pkg/shp/cmd/build/run_test.go

+)
+
+func TestStartBuildRunFollowLog(t *testing.T) {
+	//		"build.shipwright.io/name=%s,buildrun.shipwright.io/name=%s",


That might be a left over.

it is thanks

coreydaley · 2021-09-21T15:29:08Z

pkg/shp/cmd/build/run_test.go

+	buildv1alpha1 "github.com/shipwright-io/build/pkg/apis/build/v1alpha1"
+	shpfake "github.com/shipwright-io/build/pkg/client/clientset/versioned/fake"
+	"github.com/shipwright-io/cli/pkg/shp/flags"
+	"github.com/shipwright-io/cli/pkg/shp/params"
+	"github.com/spf13/cobra"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/cli-runtime/pkg/genericclioptions"
+	"k8s.io/client-go/kubernetes/fake"
+	fakekubetesting "k8s.io/client-go/testing"
+	"strings"
+	"testing"
+	"time"


nit: imports should be sorted into blocks

yep saw that and have that part of the next push as well

gabemontero · 2021-09-21T15:45:08Z

also @otaviof @coreydaley I may have figured out a better approach than the sleep on line 84 of run_test.go ... trying it out now

gabemontero · 2021-09-21T15:49:01Z

also @otaviof @coreydaley I may have figured out a better approach than the sleep on line 84 of run_test.go ... trying it out now

Yes!!! ... runtime.Gosched()

gabemontero · 2021-09-21T16:40:29Z

OK @otaviof @coreydaley I've pushed updates stemming from the comments (with still withholding on the go omega usage)

Also, I

cleaned up some ordering of things in Run based on continued race condition testing
swapped out the sleep in the test for use of Gosched per Start buildrun follow logs test #39 (comment)
added tests for the other pod phases, delete of pod/br, and cancel

PTAL / thanks

otaviof

Cool! It looks good to me!

/lgtm

gabemontero · 2021-09-22T11:45:21Z

/approve

openshift-ci · 2021-09-22T11:45:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [gabemontero]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot assigned coreydaley and HeavyWombat Sep 20, 2021

openshift-ci bot added release-note-none kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Sep 20, 2021

openshift-ci bot assigned otaviof Sep 20, 2021

openshift-ci bot requested review from adambkaplan and qu1queee September 20, 2021 21:56

otaviof requested changes Sep 21, 2021

View reviewed changes

coreydaley reviewed Sep 21, 2021

View reviewed changes

gabemontero force-pushed the start-buildrun-follow-logs-test branch from e549214 to 39826af Compare September 21, 2021 16:38

gabemontero mentioned this pull request Sep 21, 2021

Bootstrapping End-to-End Tests #40

Merged

4 tasks

add unit test for build follow logs logic

001a564

gabemontero force-pushed the start-buildrun-follow-logs-test branch from 39826af to 001a564 Compare September 21, 2021 18:10

otaviof reviewed Sep 22, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2021

openshift-merge-robot merged commit 157b479 into shipwright-io:main Sep 22, 2021

gabemontero deleted the start-buildrun-follow-logs-test branch September 22, 2021 11:56

gabemontero mentioned this pull request Sep 22, 2021

Local source code feature (using bundle images) #35

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start buildrun follow logs test #39

Start buildrun follow logs test #39

gabemontero commented Sep 20, 2021

gabemontero commented Sep 20, 2021

otaviof left a comment

otaviof Sep 21, 2021

gabemontero Sep 21, 2021

otaviof Sep 21, 2021

gabemontero Sep 21, 2021

otaviof Sep 21, 2021

gabemontero Sep 21, 2021

coreydaley Sep 21, 2021

otaviof Sep 21, 2021

gabemontero Sep 21, 2021

coreydaley Sep 21, 2021

gabemontero Sep 21, 2021

gabemontero commented Sep 21, 2021

gabemontero commented Sep 21, 2021

gabemontero commented Sep 21, 2021

otaviof left a comment

gabemontero commented Sep 22, 2021

openshift-ci bot commented Sep 22, 2021

Start buildrun follow logs test #39

Start buildrun follow logs test #39

Conversation

gabemontero commented Sep 20, 2021

Changes

Submitter Checklist

Release Notes

gabemontero commented Sep 20, 2021

otaviof left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabemontero commented Sep 21, 2021

gabemontero commented Sep 21, 2021

gabemontero commented Sep 21, 2021

otaviof left a comment

Choose a reason for hiding this comment

gabemontero commented Sep 22, 2021

openshift-ci bot commented Sep 22, 2021