From a3245d97cd6f7b29d914d05a7733e58b07d9162c Mon Sep 17 00:00:00 2001 From: Robert Gravina Date: Tue, 3 Oct 2017 12:11:40 +0900 Subject: [PATCH 1/2] Remove check for unsupplied manifest flag - CF CLI properly defaults this - update test to reflect that CF CLI also properly defaults the app path flag Signed-off-by: Douglas Blumeyer --- autopilot.go | 6 ------ autopilot_test.go | 9 +++++---- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/autopilot.go b/autopilot.go index 8cb2e73..8e39435 100644 --- a/autopilot.go +++ b/autopilot.go @@ -134,15 +134,9 @@ func ParseArgs(args []string) (string, string, string, error) { appName := args[1] - if *manifestPath == "" { - return "", "", "", ErrNoManifest - } - return appName, *manifestPath, *appPath, nil } -var ErrNoManifest = errors.New("a manifest is required to push this application") - type ApplicationRepo struct { conn plugin.CliConnection } diff --git a/autopilot_test.go b/autopilot_test.go index d1c9f76..78e31f4 100644 --- a/autopilot_test.go +++ b/autopilot_test.go @@ -36,15 +36,16 @@ var _ = Describe("Flag Parsing", func() { Expect(appPath).To(Equal("app-path")) }) - It("requires a manifest", func() { - _, _, _, err := ParseArgs( + It("does not require path or manifest flag", func() { + appName, _, _, err := ParseArgs( []string{ "zero-downtime-push", "appname", - "-p", "app-path", }, ) - Expect(err).To(MatchError(ErrNoManifest)) + Expect(err).ToNot(HaveOccurred()) + + Expect(appName).To(Equal("appname")) }) }) From 47e5f09814703ba04273ef620cdca2b7b4f6df61 Mon Sep 17 00:00:00 2001 From: Douglas Blumeyer Date: Tue, 3 Oct 2017 18:03:10 +0900 Subject: [PATCH 2/2] Allow not specifying app name if it is found in the manifest (like CF CLI). - Assumes app name does not start with '-'. - Assumes manifest is well-formed. - Uses first app in manifest if multiple are found (like CF CLI). - If name provided, and name found in manifest, name provided takes precedence (like CF CLI). - See https://github.com/contraband/autopilot/issues/32 Signed-off-by: Robert Gravina --- autopilot.go | 51 +++++++++++++++- autopilot_test.go | 61 ++++++++++++++++++- .../manifests/manifest-with-multiple-apps.yml | 4 ++ .../manifests/manifest-without-appname.yml | 3 + fixtures/manifests/manifest.yml | 3 + 5 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 fixtures/manifests/manifest-with-multiple-apps.yml create mode 100644 fixtures/manifests/manifest-without-appname.yml create mode 100644 fixtures/manifests/manifest.yml diff --git a/autopilot.go b/autopilot.go index 8e39435..cddf8cb 100644 --- a/autopilot.go +++ b/autopilot.go @@ -10,6 +10,9 @@ import ( "strings" "code.cloudfoundry.org/cli/plugin" + "code.cloudfoundry.org/cli/cf/manifest" + "code.cloudfoundry.org/cli/cf/models" + cfFlag "code.cloudfoundry.org/cli/command/flag" "github.com/contraband/autopilot/rewind" ) @@ -127,16 +130,60 @@ func ParseArgs(args []string) (string, string, string, error) { manifestPath := flags.String("f", "", "path to an application manifest") appPath := flags.String("p", "", "path to application files") - err := flags.Parse(args[2:]) + var appName string + var err error + + if args[1][0] == '-' { + err = flags.Parse(args[1:]) + appName, err = GetAppNameFromManifest(*manifestPath) + } else { + err = flags.Parse(args[2:]) + appName = args[1] + } + if err != nil { return "", "", "", err } - appName := args[1] + if appName == "" { + return "", "", "", ErrNoAppName + } return appName, *manifestPath, *appPath, nil } +func GetAppNameFromManifest(manifestPath string) (string, error) { + var err error + + var path = cfFlag.PathWithExistenceCheck(manifestPath) + err = path.UnmarshalFlag(manifestPath) + if err != nil { + return "", err + } + + var m *manifest.Manifest + var dr = manifest.NewDiskRepository() + m, err = dr.ReadManifest(manifestPath) + if err != nil { + return "", err + } + + var apps []models.AppParams + apps, err = m.Applications() + if err != nil { + return "", err + } + + var name = apps[0].Name + if name == nil { + return "", ErrNoAppName + } + + return *name, nil +} + +var ErrNoAppName = errors.New("a name is required to push this application") + type ApplicationRepo struct { conn plugin.CliConnection } diff --git a/autopilot_test.go b/autopilot_test.go index 78e31f4..b2d271f 100644 --- a/autopilot_test.go +++ b/autopilot_test.go @@ -25,14 +25,14 @@ var _ = Describe("Flag Parsing", func() { []string{ "zero-downtime-push", "appname", - "-f", "manifest-path", + "-f", "./fixtures/manifests/manifest.yml", "-p", "app-path", }, ) Expect(err).ToNot(HaveOccurred()) Expect(appName).To(Equal("appname")) - Expect(manifestPath).To(Equal("manifest-path")) + Expect(manifestPath).To(Equal("./fixtures/manifests/manifest.yml")) Expect(appPath).To(Equal("app-path")) }) @@ -47,6 +47,63 @@ var _ = Describe("Flag Parsing", func() { Expect(appName).To(Equal("appname")) }) + + It("does not require app name if provided in the manifest", func() { + appName, manifestPath, _, err := ParseArgs( + []string{ + "zero-downtime-push", + "-f", "./fixtures/manifests/manifest.yml", + }, + ) + Expect(err).ToNot(HaveOccurred()) + + Expect(appName).To(Equal("appname-from-manifest")) + Expect(manifestPath).To(Equal("./fixtures/manifests/manifest.yml")) + }) + + It("errors if no app name is provided (as either a flag or in the manifest)", func() { + _, _, _, err := ParseArgs( + []string{ + "zero-downtime-push", + "-f", "./fixtures/manifests/manifest-without-appname.yml", + }, + ) + Expect(err).To(MatchError(ErrNoAppName)) + }) + + It("the provided app flag takes precedence over an app name in the manifest", func() { + appName, manifestPath, _, err := ParseArgs( + []string{ + "zero-downtime-push", + "appname-from-flag", + "-f", "./fixtures/manifests/manifest.yml", + }, + ) + Expect(err).ToNot(HaveOccurred()) + + Expect(appName).To(Equal("appname-from-flag")) + Expect(manifestPath).To(Equal("./fixtures/manifests/manifest.yml")) + }) + + It("defaults to finding the name from the first app in the manifest", func() { + appName, _, _, _ := ParseArgs( + []string{ + "zero-downtime-push", + "-f", "./fixtures/manifests/manifest-with-multiple-apps.yml", + }, + ) + Expect(appName).To(Equal("first-appname")) + }) + + It("errors if manifest path is bad", func() { + _, _, _, err := ParseArgs( + []string{ + "zero-downtime-push", + "-f", "./fixtures/manifests/nonexistent-manifest.yml", + }, + ) + Expect(err).To(HaveOccurred()) + }) }) var _ = Describe("ApplicationRepo", func() { diff --git a/fixtures/manifests/manifest-with-multiple-apps.yml b/fixtures/manifests/manifest-with-multiple-apps.yml new file mode 100644 index 0000000..8e186da --- /dev/null +++ b/fixtures/manifests/manifest-with-multiple-apps.yml @@ -0,0 +1,4 @@ +--- +applications: +- name: first-appname +- name: second-appname \ No newline at end of file diff --git a/fixtures/manifests/manifest-without-appname.yml b/fixtures/manifests/manifest-without-appname.yml new file mode 100644 index 0000000..a558ff0 --- /dev/null +++ b/fixtures/manifests/manifest-without-appname.yml @@ -0,0 +1,3 @@ +--- +applications: +- memory: 256M diff --git a/fixtures/manifests/manifest.yml b/fixtures/manifests/manifest.yml new file mode 100644 index 0000000..d8af2bf --- /dev/null +++ b/fixtures/manifests/manifest.yml @@ -0,0 +1,3 @@ +--- +applications: +- name: appname-from-manifest