Fix procfile (#109)

* attempt to fix deploy from source and Procfile issue * Fix tests * Add error message for when using procfile with deploy from source * Add test for flag logic * Fix initial deployment, procfile flag logic not fully functional * Remove procfile flag * Prevent the user from building from src if Procfile not in src root * Fix tests and implement logic for creating procfile * Add comment detailing pack peculiarities Co-authored-by: Vivek Pandey <vivek.pandey@gmail.com>
theketchio · Jul 1, 2021 · 7b54803 · 7b54803
1 parent 59f4a8c
commit 7b54803
Show file tree

Hide file tree

Showing 8 changed files with 89 additions and 42 deletions.
diff --git a/cmd/ketch/app_deploy.go b/cmd/ketch/app_deploy.go
@@ -12,16 +12,17 @@ Roll out a new version of an application with an image.
 
 Deploy from source code. <source> is path to source code. The image in this case is required
 and will be built using the selected source code and builder and will be used to deploy the app.
+
+Similarly, the source path's root directory must contain Procfile as specified by pack.
+Details about Procfile conventions can be found here: https://devcenter.heroku.com/articles/procfile
   ketch app deploy <app name> <source> -i myregistry/myimage:latest
 
-  Ketch looks for Procfile and ketch.yaml inside the source directory by default
-  but you can provide a custom path with --procfile or --ketch-yaml.
+  Ketch looks for ketch.yaml inside the source directory by default
+  but you can provide a custom path with --ketch-yaml.
 
 Deploy from an image:
   ketch app deploy <app name> -i myregistry/myimage:latest
 
-  Ketch uses the image's cmd and entrypoint but you can redefine what exactly to run with --procfile.
-
 `
 )
 
@@ -52,7 +53,6 @@ func newAppDeployCmd(cfg config, params *deploy.Services, configDefaultBuilder s
 	cmd.Flags().StringVarP(&options.Image, deploy.FlagImage, deploy.FlagImageShort, "", "Name of the image to be deployed.")
 	cmd.Flags().StringVar(&options.KetchYamlFileName, deploy.FlagKetchYaml, "", "Path to ketch.yaml.")
 
-	cmd.Flags().StringVar(&options.ProcfileFileName, deploy.FlagProcFile, "", "Path to procfile.")
 	cmd.Flags().BoolVar(&options.StrictKetchYamlDecoding, deploy.FlagStrict, false, "Enforces strict decoding of ketch.yaml.")
 	cmd.Flags().IntVar(&options.Steps, deploy.FlagSteps, 0, "Number of steps for a canary deployment.")
 	cmd.Flags().StringVar(&options.StepTimeInterval, deploy.FlagStepInterval, "", "Time interval between canary deployment steps. Supported min: m, hour:h, second:s. ex. 1m, 60s, 1h.")

diff --git a/cmd/ketch/app_deploy_test.go b/cmd/ketch/app_deploy_test.go
@@ -128,7 +128,8 @@ func getImageConfig(ctx context.Context, args deploy.ImageConfigRequest) (*regis
 	}, nil
 }
 
-var ketchYaml string = `
+var (
+	ketchYaml string = `
 kubernetes:
   processes:
     web:
@@ -145,6 +146,11 @@ kubernetes:
     worker-2:
       ports: []
 `
+	procfile string = `
+web: python app.py
+worker: python app.py
+`
+)
 
 func TestNewCommand(t *testing.T) {
 	tt := []struct {
@@ -168,6 +174,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			validate: func(t *testing.T, m deploy.Client) {
 				mock, ok := m.(*mockClient)
@@ -199,6 +206,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			validate: func(t *testing.T, m deploy.Client) {
 				mock, ok := m.(*mockClient)
@@ -230,6 +238,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			validate: func(t *testing.T, m deploy.Client) {
 				mock, ok := m.(*mockClient)
@@ -263,6 +272,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			userDefault: "newDefault",
 			validate: func(t *testing.T, m deploy.Client) {
@@ -296,6 +306,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			userDefault: "newDefault",
 			validate: func(t *testing.T, m deploy.Client) {
@@ -343,6 +354,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			validate: func(t *testing.T, m deploy.Client) {
 				mock, ok := m.(*mockClient)
@@ -381,6 +393,7 @@ func TestNewCommand(t *testing.T) {
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
 				require.Nil(t, ioutil.WriteFile("src/ketch.yaml", []byte(ketchYaml), 0600))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			validate: func(t *testing.T, m deploy.Client) {
 				mock, ok := m.(*mockClient)
@@ -426,6 +439,7 @@ func TestNewCommand(t *testing.T) {
 				require.Nil(t, os.MkdirAll(path.Join(dir, "src/include2"), 0700))
 				require.Nil(t, os.Chdir(dir))
 				require.Nil(t, ioutil.WriteFile("config/ketch.yaml", []byte(ketchYaml), 0600))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			validate: func(t *testing.T, m deploy.Client) {
 				mock, ok := m.(*mockClient)
@@ -461,6 +475,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			validate: func(t *testing.T, m deploy.Client) {
 				mock, ok := m.(*mockClient)
@@ -562,6 +577,7 @@ func TestNewCommand(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
 				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
 			},
 			params: &deploy.Services{
 				Client: func() *mockClient {
@@ -589,6 +605,36 @@ func TestNewCommand(t *testing.T) {
 				"--image", "shipa/go-sample:latest",
 				"--env", "foo=bar,bobbdobbs",
 			},
+			setup: func(t *testing.T) {
+				dir := t.TempDir()
+				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))
+				require.Nil(t, os.Chdir(dir))
+				require.Nil(t, ioutil.WriteFile("src/Procfile", []byte(procfile), 0600))
+			},
+			params: &deploy.Services{
+				Client: func() *mockClient {
+					m := newMockClient()
+					m.get[1] = func(_ *mockClient, _ runtime.Object) error {
+						return errors.NewNotFound(v1.Resource(""), "")
+					}
+					return m
+				}(),
+				KubeClient:     fake.NewSimpleClientset(),
+				Builder:        build.GetSourceHandler(&packMocker{}),
+				GetImageConfig: getImageConfig,
+				Wait:           nil,
+				Writer:         &bytes.Buffer{},
+			},
+		},
+		{
+			name:      "missing Procfile in src",
+			wantError: true,
+			arguments: []string{
+				"myapp",
+				"src",
+				"--framework", "myframework",
+				"--image", "shipa/go-sample:latest",
+			},
 			setup: func(t *testing.T) {
 				dir := t.TempDir()
 				require.Nil(t, os.Mkdir(path.Join(dir, "src"), 0700))

diff --git a/internal/chart/application_chart.go b/internal/chart/application_chart.go
@@ -147,7 +147,6 @@ func New(application *ketchv1.App, framework *ketchv1.Framework, opts ...Option)
 		for _, processSpec := range deploymentSpec.Processes {
 			name := processSpec.Name
 			isRoutable := procfile.IsRoutable(name)
-
 			process, err := newProcess(name, isRoutable,
 				withCmd(c.procfile.Processes[name]),
 				withUnits(processSpec.Units),

diff --git a/internal/chart/procfile.go b/internal/chart/procfile.go
@@ -45,15 +45,20 @@ func (p *Procfile) SortedNames() []string {
 }
 
 // ParseProcfile parses the content of Procfile and returns a Procfile instance.
+// this function should only be called when building from source, since deploying
+// from image does not allow a user to specify a Procfile
 func ParseProcfile(content string) (*Procfile, error) {
 	procfile := strings.Split(content, "\n")
 	processes := make(map[string][]string, len(procfile))
 	var names []string
 	for _, process := range procfile {
 		if p := procfileRegex.FindStringSubmatch(process); p != nil {
 			name := p[1]
-			cmd := p[2]
-			processes[name] = []string{strings.TrimSpace(cmd)}
+			// inside the docker image created by pack, executables specified as the names
+			// in the procfile will be added to /cnb/process. These executables run the commands
+			// specified in the procfile. Trying to run the commands as they are in the Procfile
+			// will result in an executable file not found in $PATH: unknown error
+			processes[name] = []string{strings.TrimSpace(name)}
 			names = append(names, name)
 		}
 	}

diff --git a/internal/chart/procfile_test.go b/internal/chart/procfile_test.go
@@ -21,7 +21,7 @@ func TestParseProcfile(t *testing.T) {
 			content: "web: command arg1 arg2",
 			want: &Procfile{
 				Processes: map[string][]string{
-					"web": {"command arg1 arg2"},
+					"web": {"web"},
 				},
 				RoutableProcessName: "web",
 			},
@@ -31,7 +31,7 @@ func TestParseProcfile(t *testing.T) {
 			content: "long-command-name: command arg1 arg2",
 			want: &Procfile{
 				Processes: map[string][]string{
-					"long-command-name": {"command arg1 arg2"},
+					"long-command-name": {"long-command-name"},
 				},
 				RoutableProcessName: "long-command-name",
 			},
@@ -41,8 +41,8 @@ func TestParseProcfile(t *testing.T) {
 			content: "web: command arg1 arg2\nworker: celery worker",
 			want: &Procfile{
 				Processes: map[string][]string{
-					"web":    {"command arg1 arg2"},
-					"worker": {"celery worker"},
+					"web":    {"web"},
+					"worker": {"worker"},
 				},
 				RoutableProcessName: "web",
 			},
@@ -52,8 +52,8 @@ func TestParseProcfile(t *testing.T) {
 			content: "worker: command arg1 arg2\n\r\nabc: abc-arg1 abc-arg2",
 			want: &Procfile{
 				Processes: map[string][]string{
-					"worker": {"command arg1 arg2"},
-					"abc":    {"abc-arg1 abc-arg2"},
+					"worker": {"worker"},
+					"abc":    {"abc"},
 				},
 				RoutableProcessName: "abc",
 			},
@@ -63,9 +63,9 @@ func TestParseProcfile(t *testing.T) {
 			content: "bbb: bbb-command\n\r\nzzz: zzz-command\r\naaa: aaa-command",
 			want: &Procfile{
 				Processes: map[string][]string{
-					"aaa": {"aaa-command"},
-					"zzz": {"zzz-command"},
-					"bbb": {"bbb-command"},
+					"aaa": {"aaa"},
+					"zzz": {"zzz"},
+					"bbb": {"bbb"},
 				},
 				RoutableProcessName: "aaa",
 			},
@@ -75,19 +75,19 @@ func TestParseProcfile(t *testing.T) {
 			content: "bbb: bbb-command\n# some comment\n\nzzz: zzz-command\r\naaa: aaa-command\n # another comment",
 			want: &Procfile{
 				Processes: map[string][]string{
-					"aaa": {"aaa-command"},
-					"zzz": {"zzz-command"},
-					"bbb": {"bbb-command"},
+					"aaa": {"aaa"},
+					"zzz": {"zzz"},
+					"bbb": {"bbb"},
 				},
 				RoutableProcessName: "aaa",
 			},
 		},
 		{
-			name:    "ingore broken lines",
+			name:    "ignore broken lines",
 			content: "b,bb: bbb-command\n\r\n: zzz-command\r\naaa: aaa-command",
 			want: &Procfile{
 				Processes: map[string][]string{
-					"aaa": {"aaa-command"},
+					"aaa": {"aaa"},
 				},
 				RoutableProcessName: "aaa",
 			},

diff --git a/internal/deploy/deploy.go b/internal/deploy/deploy.go
@@ -5,6 +5,7 @@ package deploy
 import (
 	"context"
 	"fmt"
+	"path"
 	"time"
 
 	registryv1 "github.com/google/go-containerregistry/pkg/v1"
@@ -25,6 +26,7 @@ const (
 	defaultTrafficWeight = 100
 	minimumSteps         = 2
 	maximumSteps         = 100
+	defaultProcFile      = "Procfile"
 )
 
 // Client represents go sdk k8s client operations that we need.
@@ -184,6 +186,7 @@ func deployFromSource(ctx context.Context, svc *Services, app *ketchv1.App, para
 
 	image, _ := params.getImage()
 	sourcePath, _ := params.getSourceDirectory()
+	sourceProcFilePath := path.Join(sourcePath, defaultProcFile)
 
 	if err := svc.Builder(
 		ctx,
@@ -209,7 +212,7 @@ func deployFromSource(ctx context.Context, svc *Services, app *ketchv1.App, para
 		return err
 	}
 
-	procfile, err := makeProcfile(imgConfig, params)
+	procfile, err := makeProcfile(nil, sourceProcFilePath)
 	if err != nil {
 		return err
 	}
@@ -266,7 +269,7 @@ func deployFromImage(ctx context.Context, svc *Services, app *ketchv1.App, param
 		return err
 	}
 
-	procfile, err := makeProcfile(imgConfig, params)
+	procfile, err := makeProcfile(imgConfig, "")
 	if err != nil {
 		return err
 	}
@@ -298,12 +301,13 @@ func deployFromImage(ctx context.Context, svc *Services, app *ketchv1.App, param
 	return nil
 }
 
-func makeProcfile(cfg *registryv1.ConfigFile, params *ChangeSet) (*chart.Procfile, error) {
-	procFileName, err := params.getProcfileName()
-	if !isMissing(err) {
+func makeProcfile(cfg *registryv1.ConfigFile, procFileName string) (*chart.Procfile, error) {
+	if procFileName != "" {
+		// validating of path handled by validateSourceDeploy function
 		return chart.NewProcfile(procFileName)
 	}
 
+	// no procfile (not building from source)
 	cmds := append(cfg.Config.Entrypoint, cfg.Config.Cmd...)
 	if len(cmds) == 0 {
 		return nil, fmt.Errorf("can't use image, no entrypoint or commands")

diff --git a/internal/deploy/parameters.go b/internal/deploy/parameters.go
@@ -25,7 +25,6 @@ const (
 	FlagApp            = "app"
 	FlagImage          = "image"
 	FlagKetchYaml      = "ketch-yaml"
-	FlagProcFile       = "procfile"
 	FlagStrict         = "strict"
 	FlagSteps          = "steps"
 	FlagStepInterval   = "step-interval"
@@ -75,7 +74,6 @@ type Options struct {
 	AppName                 string
 	Image                   string
 	KetchYamlFileName       string
-	ProcfileFileName        string
 	StrictKetchYamlDecoding bool
 	Steps                   int
 	StepTimeInterval        string
@@ -98,7 +96,6 @@ type ChangeSet struct {
 	sourcePath           *string
 	image                *string
 	ketchYamlFileName    *string
-	procfileFileName     *string
 	steps                *int
 	stepTimeInterval     *string
 	wait                 *bool
@@ -130,9 +127,6 @@ func (o Options) GetChangeSet(flags *pflag.FlagSet) *ChangeSet {
 		FlagKetchYaml: func(c *ChangeSet) {
 			c.ketchYamlFileName = &o.KetchYamlFileName
 		},
-		FlagProcFile: func(c *ChangeSet) {
-			c.procfileFileName = &o.ProcfileFileName
-		},
 		FlagSteps: func(c *ChangeSet) {
 			c.steps = &o.Steps
 		},
@@ -172,13 +166,6 @@ func (o Options) GetChangeSet(flags *pflag.FlagSet) *ChangeSet {
 	return &cs
 }
 
-func (c *ChangeSet) getProcfileName() (string, error) {
-	if c.procfileFileName == nil {
-		return "", newMissingError(FlagProcFile)
-	}
-	return *c.procfileFileName, nil
-}
-
 func (c *ChangeSet) getDescription() (string, error) {
 	if c.description == nil {
 		return "", newMissingError(FlagDescription)