Feat/world class dev experience

[wayneadams]

These changes create an easy and straightforward user experience for creating tiles and testing metadata/manifest changes within the tile.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

[crhntr]

I thought I’d add a manifest test to Hello Tile (my tiny little tile for testing/learning). It works so well!

If you use it instead of TAS in the tests for kiln test, you can get kiln test (with a simpler input) coverage and not publicly publish a revision of TAS. We already use hello-tile for testing in the acceptance tests.

I found that git does not fetch planitest (via go get) using ssh… it tries to use HTTPS and fails. This is not an issue for TAS because it has a vendor directory. If kiln test runs something like this git config --global url.ssh://git@github.com/.insteadOf https://github.com/ in the container, go/git might work better with fetching private repositories. To work around the issue in Hello Tile, I added a step to the ReadMe to run go vendor before running Kiln Bake (I don’t want to make planitest public (even in a vendor directory) under in a personal repository. I'd suggest we move planitest into the kiln repo and start make it better. The API could be improved to make testing tiles better. The amount of boilerplate needed to get the manifests tests working was not terrible but could be improved.

Check it out here: https://github.com/crhntr/hello-tile
It is mirrored (internally) here: https://github.com/pivotal/hello-tile

Here is the test output (I am not sure where the strange prefix characters are coming from... might be a problem with my code, if not we might want to add an issue):

$ kiln test
Info: Checking for the latest ops-manager image...
Info: Building / restoring cached docker image. This may take several minutes during updates to Ops Manager or the first run...
Info: {"stream":"Successfully tagged dont_push_me_vmware_confidential:123\n"}
Info: Mounting  /Users/hunterch/workspace and testing hello-tile
cd /tas/hello-tile/test/manifest && PRODUCT=hello-tile RENDERER=ops-manifest ginkgo -r -slowSpecThreshold 15
=== RUN   TestManifest
Reading release manifests...
-Reading stemcell criteria from Kilnfile.lock
Encoding icon...
=== RUN   TestManifest/port
$=== RUN   TestManifest/port/Default
(=== RUN   TestManifest/port/Not_Default
--- PASS: TestManifest (2.20s)
(    --- PASS: TestManifest/port (1.84s)
4        --- PASS: TestManifest/port/Default (0.86s)
8        --- PASS: TestManifest/port/Not_Default (0.98s)
PASS

#Ginkgo ran 1 suite in 5.593694669s
Test Suite Passed
$

[crhntr]

The suggestions for "CodeQL" comments that show during a code review are pretty good suggestions. There are a bunch of issues so take a glance but some might be hard to properly test.

[crhntr]

I was taking some time understanding the tests before switching to using hello-tile. I was looking in particular for whether switching would cause a loss over coverage. It suspect a switch to hello-tile will be okay but before I did so, I decided to propose a few test organization changes.

In this suggested code, I added a few helper functions to make the happy path tests dryer and put the tile tests in a Describe.

package commands_test

import (
	"bytes"
	"context"
	"errors"
	"fmt"
	"io"
	"log"
	"os"
	"strings"

	"github.com/docker/docker/api/types"
	"github.com/docker/docker/api/types/container"
	"github.com/docker/docker/api/types/strslice"
	"github.com/docker/docker/pkg/stdcopy"
	. "github.com/onsi/ginkgo"
	. "github.com/onsi/gomega"
	"github.com/onsi/gomega/format"

	"github.com/pivotal-cf/kiln/internal/commands"
	"github.com/pivotal-cf/kiln/internal/commands/fakes"
)

func init() {
	format.MaxLength = 100000
}

var _ = FDescribe("kiln test docker", func() {
	Context("locally missing docker image is built", func() {
		var (
			ctx    context.Context
			writer strings.Builder
			logger *log.Logger
		)

		BeforeEach(func() {
			ctx = context.Background()
			logger = log.New(&writer, "", 0)
		})

		Describe("successful tile builds", func() {
			var (
				fakeMobyClient  *fakes.MobyClient
				fakeSshProvider *fakes.SshProvider
			)

			BeforeEach(func() {
				writePasswordToStdIn(GinkgoT())
				setupFakeMobyClient()
				fakeMobyClient = setupFakeMobyClient()
				fakeSshProvider = setupSSHThing()
			})

			It("runs the tests for ist", func() {
				subjectUnderTest := commands.NewManifestTest(logger, ctx, fakeMobyClient, fakeSshProvider)

				err := subjectUnderTest.Execute([]string{"--tile-path", "tas_fake/ist", "--ginkgo-manifest-flags", "-r -slowSpecThreshold 1"})
				Expect(err).To(BeNil())

				_, config, _, _, _, _ := fakeMobyClient.ContainerCreateArgsForCall(0)
				Expect(len(config.Env)).To(Equal(2))
				Expect(config.Env[0]).To(Equal("TAS_METADATA_PATH=/tas/ist/test/manifest/fixtures/tas_metadata.yml"))
				Expect(config.Env[1]).To(Equal("TAS_CONFIG_FILE=/tas/ist/test/manifest/fixtures/tas_config.yml"))

				dockerCmd := fmt.Sprintf("cd /tas/%s/test/manifest && PRODUCT=ist RENDERER=ops-manifest ginkgo %s", "ist", "-r -slowSpecThreshold 1")
				Expect(config.Cmd).To(Equal(strslice.StrSlice{"/bin/bash", "-c", dockerCmd}))

				GinkgoT().Log(writer.String())
				Expect((&writer).String()).To(ContainSubstring("tagged dont_push_me_vmware_confidential:123"))
				Expect((&writer).String()).To(ContainSubstring("Building / restoring cached docker image"))
			})

			It("runs the tests for tas", func() {
				subjectUnderTest := commands.NewManifestTest(logger, ctx, fakeMobyClient, fakeSshProvider)

				err := subjectUnderTest.Execute([]string{"--tile-path", "tas_fake/tas"})
				Expect(err).To(BeNil())

				_, config, _, _, _, _ := fakeMobyClient.ContainerCreateArgsForCall(0)
				Expect(len(config.Env)).To(Equal(0))

				GinkgoT().Log(writer.String())
				Expect((&writer).String()).To(ContainSubstring("tagged dont_push_me_vmware_confidential:123"))
				Expect((&writer).String()).To(ContainSubstring("Building / restoring cached docker image"))
			})
		})

		It("exits with an error if docker isn't running", func() {
			fakeMobyClient := &fakes.MobyClient{}
			fakeMobyClient.PingReturns(types.Ping{}, errors.New("docker not running"))
			fakeSshThinger := fakes.SshProvider{}
			fakeSshThinger.NeedsKeysReturns(false, nil)
			subjectUnderTest := commands.NewManifestTest(logger, ctx, fakeMobyClient, &fakeSshThinger)
			err := subjectUnderTest.Execute([]string{"tas_fake"})
			Expect(err).To(HaveOccurred())
			Expect(err.Error()).To(ContainSubstring("Docker daemon is not running"))
		})
	})

	Context("logs output from container", func() {
		It("logs when the manifest tests complete successfully", func() {
			fakeMobyClient := &fakes.MobyClient{}

			ctx := context.Background()
			fakeMobyClient.PingReturns(types.Ping{}, nil)
			r, w := io.Pipe()
			_ = w.Close()
			fakeConn := &fakes.Conn{R: r, W: stdcopy.NewStdWriter(io.Discard, stdcopy.Stdout)}
			fakeMobyClient.DialHijackReturns(fakeConn, nil)
			output := "Successfully built 1234"
			rc := io.NopCloser(strings.NewReader(fmt.Sprintf(`{"error": "", "message": "%s"}`, output)))
			imageBuildResponse := types.ImageBuildResponse{
				Body: rc,
			}
			fakeMobyClient.ImageBuildReturns(imageBuildResponse, nil)
			createResp := container.CreateResponse{
				ID: "some id",
			}
			fakeMobyClient.ContainerCreateReturns(createResp, nil)
			var logLogOutput bytes.Buffer
			logOut := log.New(&logLogOutput, "", 0)

			rcLog := io.NopCloser(strings.NewReader(`"manifest tests completed successfully"`))
			fakeMobyClient.ContainerLogsReturns(rcLog, nil)
			fakeMobyClient.ContainerStartReturns(nil)
			fakeSshThinger := fakes.SshProvider{}
			fakeSshThinger.NeedsKeysReturns(false, nil)
			subjectUnderTest := commands.NewManifestTest(logOut, ctx, fakeMobyClient, &fakeSshThinger)
			fakeMobyClient.ContainerCreateReturns(createResp, nil)
			responses := make(chan container.WaitResponse)
			go func() {
				responses <- container.WaitResponse{
					Error:      nil,
					StatusCode: 0,
				}
			}()
			fakeMobyClient.ContainerWaitReturns(responses, nil)

			err := subjectUnderTest.Execute([]string{"--tile-path", "tas_fake/tas"})
			Expect(err).To(BeNil())

			Expect(logLogOutput.String()).To(ContainSubstring("manifest tests completed successfully"))
		})
	})
})

func setupFakeMobyClient() *fakes.MobyClient {
	fakeMobyClient := &fakes.MobyClient{}
	fakeMobyClient.PingReturns(types.Ping{}, nil)
	r, w := io.Pipe()
	_ = w.Close()
	fakeConn := &fakes.Conn{R: r, W: stdcopy.NewStdWriter(io.Discard, stdcopy.Stdout)}
	fakeMobyClient.DialHijackReturns(fakeConn, nil)

	rc := io.NopCloser(strings.NewReader(`{"error": "", "message": "tagged dont_push_me_vmware_confidential:123"}`))
	imageBuildResponse := types.ImageBuildResponse{
		Body: rc,
	}
	fakeMobyClient.ImageBuildReturns(imageBuildResponse, nil)
	createResp := container.CreateResponse{
		ID: "some id",
	}
	fakeMobyClient.ContainerCreateReturns(createResp, nil)
	responses := make(chan container.WaitResponse)
	go func() {
		responses <- container.WaitResponse{
			Error:      nil,
			StatusCode: 0,
		}
	}()
	fakeMobyClient.ContainerWaitReturns(responses, nil)
	rcLog := io.NopCloser(strings.NewReader(`{"error": "", "message": "manifest tests completed successfully"}"`))
	fakeMobyClient.ContainerLogsReturns(rcLog, nil)
	fakeMobyClient.ContainerStartReturns(nil)
	return fakeMobyClient
}

type helperTestingT interface {
	Helper()
	Cleanup(func())
	TempDir() string
	Fatal(args ...interface{})
	Name() string
}

func writePasswordToStdIn(t helperTestingT) {
	t.Helper()
	oldStdin := os.Stdin
	t.Cleanup(func() {
		os.Stdin = oldStdin
	})
	passwd := "password\n"
	content := []byte(passwd)
	temporaryFile, err := os.CreateTemp(t.TempDir(), t.Name())
	if err != nil {
		t.Fatal(err)
	}
	_, err = temporaryFile.Write(content)
	if err != nil {
		t.Fatal(err)
	}
	_, err = temporaryFile.Seek(0, 0)
	if err != nil {
		t.Fatal(err)
	}
	os.Stdin = temporaryFile
}

func setupSSHThing() *fakes.SshProvider {
	fakeSSHProvider := fakes.SshProvider{}
	fakeSSHProvider.NeedsKeysReturns(false, nil)
	return &fakeSSHProvider
}

[crhntr]

I noticed the commands test "kiln test docker" does work to modify the test process stdin and stdout. This is hard to read and is brittle (because those exported variables are effectively globals). The command maybe can have fields for Stdin, Stdout, and Stderr and those can default to the process file descriptors in the command constructor but in tests can be assigned over with in-memory buffers. This would make tests easier to read. Another option would be to call the command via Exec and manipulate stdio on the Cmd struct but this would cause the tests to appear to loose test coverage and debuggability.

There is a section in this article where the risks of modifying standard IO are discussed (search for Stdin).

[crhntr]

This is a related issue: #356

[crhntr]

Please check the error, given this is in main, I'd say using log.Fatal is okay (log.Fatal calls os.Exit(1) and IMO exit should only be called from main.

[crhntr]

It might be nice if this was logged to a passed in logger instead of using the default logger. Some of the code uses the default logger other parts take a logger. At the very least, we should standardize one way or another.

[crhntr]

This might be a good longer chore (to pass loggers properly).

[crhntr]

(optional) Consider vertical alignment of tags. It is a bit awkward but it makes it more readable.

[crhntr]

(not important)

I like labels especially for stuff like this but have gotten code review feedback that they can be hard to read. One thing that makes this probably okay is the distance from the continue/break is <20 loc. As a kindness, you may consider pulling the inner loop into a function and return.