From ad28922871fd287037c2896fac498e4e92852610 Mon Sep 17 00:00:00 2001
From: smruthi2187 <34555664+smruthi2187@users.noreply.github.com>
Date: Thu, 16 Aug 2018 22:53:37 -0700
Subject: [PATCH] Fix for #662: avoid unnecessary builds (#866)

---
 fission/function.go | 18 ++++++++++++++++--
 fission/package.go  | 30 ++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/fission/function.go b/fission/function.go
index 22f5d03f..59600b60 100644
--- a/fission/function.go
+++ b/fission/function.go
@@ -394,6 +394,17 @@ func fnUpdate(c *cli.Context) error {
 
 	envName := c.String("env")
 	envNamespace := c.String("envNamespace")
+	// if the new env specified is the same as the old one, no need to update package
+	// same is true for all update parameters, but, for now, we dont check all of them - because, its ok to
+	// re-write the object with same old values, we just end up getting a new resource version for the object.
+	if len(envName) > 0 && envName == function.Spec.Environment.Name {
+		envName = ""
+	}
+
+	if envNamespace == function.Spec.Environment.Namespace {
+		envNamespace = ""
+	}
+
 	deployArchiveName := c.String("code")
 	if len(deployArchiveName) == 0 {
 		deployArchiveName = c.String("deploy")
@@ -455,6 +466,9 @@ func fnUpdate(c *cli.Context) error {
 
 	if len(envName) > 0 {
 		function.Spec.Environment.Name = envName
+	}
+
+	if len(envNamespace) > 0 {
 		function.Spec.Environment.Namespace = envNamespace
 	}
 
@@ -473,7 +487,7 @@ func fnUpdate(c *cli.Context) error {
 
 	pkgMetadata := &pkg.Metadata
 
-	if len(deployArchiveName) != 0 || len(srcArchiveName) != 0 || len(buildcmd) != 0 || len(envName) != 0 {
+	if len(deployArchiveName) != 0 || len(srcArchiveName) != 0 || len(buildcmd) != 0 || len(envName) != 0 || len(envNamespace) != 0 {
 		fnList, err := getFunctionsByPackage(client, pkg.Metadata.Name, pkg.Metadata.Namespace)
 		checkErr(err, "get function list")
 
@@ -481,7 +495,7 @@ func fnUpdate(c *cli.Context) error {
 			log.Fatal("Package is used by multiple functions, use --force to force update")
 		}
 
-		pkgMetadata = updatePackage(client, pkg, envName, envNamespace, srcArchiveName, deployArchiveName, buildcmd, false)
+		pkgMetadata, err = updatePackage(client, pkg, envName, envNamespace, srcArchiveName, deployArchiveName, buildcmd, false)
 		checkErr(err, fmt.Sprintf("update package '%v'", pkgName))
 
 		fmt.Printf("package '%v' updated\n", pkgMetadata.GetName())
diff --git a/fission/package.go b/fission/package.go
index 2ef333df..3f65c255 100644
--- a/fission/package.go
+++ b/fission/package.go
@@ -117,6 +117,17 @@ func pkgUpdate(c *cli.Context) error {
 	})
 	checkErr(err, "get package")
 
+	// if the new env specified is the same as the old one, no need to update package
+	// same is true for all update parameters, but, for now, we dont check all of them - because, its ok to
+	// re-write the object with same old values, we just end up getting a new resource version for the object.
+	if len(envName) > 0 && envName == pkg.Spec.Environment.Name {
+		envName = ""
+	}
+
+	if envNamespace == pkg.Spec.Environment.Namespace {
+		envNamespace = ""
+	}
+
 	fnList, err := getFunctionsByPackage(client, pkg.Metadata.Name, pkg.Metadata.Namespace)
 	checkErr(err, "get function list")
 
@@ -124,8 +135,11 @@ func pkgUpdate(c *cli.Context) error {
 		log.Fatal("Package is used by multiple functions, use --force to force update")
 	}
 
-	newPkgMeta := updatePackage(client, pkg,
+	newPkgMeta, err := updatePackage(client, pkg,
 		envName, envNamespace, srcArchiveName, deployArchiveName, buildcmd, false)
+	if err != nil {
+		checkErr(err, "update package")
+	}
 
 	// update resource version of package reference of functions that shared the same package
 	for _, fn := range fnList {
@@ -140,13 +154,17 @@ func pkgUpdate(c *cli.Context) error {
 }
 
 func updatePackage(client *client.Client, pkg *crd.Package, envName, envNamespace,
-	srcArchiveName, deployArchiveName, buildcmd string, forceRebuild bool) *metav1.ObjectMeta {
+	srcArchiveName, deployArchiveName, buildcmd string, forceRebuild bool) (*metav1.ObjectMeta, error) {
 
 	var srcArchiveMetadata, deployArchiveMetadata *fission.Archive
 	needToBuild := false
 
 	if len(envName) > 0 {
 		pkg.Spec.Environment.Name = envName
+		needToBuild = true
+	}
+
+	if len(envNamespace) > 0 {
 		pkg.Spec.Environment.Namespace = envNamespace
 		needToBuild = true
 	}
@@ -165,6 +183,9 @@ func updatePackage(client *client.Client, pkg *crd.Package, envName, envNamespac
 	if len(deployArchiveName) > 0 {
 		deployArchiveMetadata = createArchive(client, deployArchiveName, "")
 		pkg.Spec.Deployment = *deployArchiveMetadata
+		// Users may update the env, envNS and deploy archive at the same time,
+		// but without the source archive. In this case, we should set needToBuild to false
+		needToBuild = false
 	}
 
 	// Set package as pending status when needToBuild is true
@@ -178,7 +199,7 @@ func updatePackage(client *client.Client, pkg *crd.Package, envName, envNamespac
 	newPkgMeta, err := client.PackageUpdate(pkg)
 	checkErr(err, "update package")
 
-	return newPkgMeta
+	return newPkgMeta, err
 }
 
 func pkgSourceGet(c *cli.Context) error {
@@ -408,7 +429,8 @@ func pkgRebuild(c *cli.Context) error {
 			pkg.Metadata.Name, fission.BuildStatusFailed))
 	}
 
-	updatePackage(client, pkg, "", "", "", "", "", true)
+	_, err = updatePackage(client, pkg, "", "", "", "", "", true)
+	checkErr(err, "update package")
 
 	fmt.Printf("Retrying build for pkg %v. Use \"fission pkg info --name %v\" to view status.\n", pkg.Metadata.Name, pkg.Metadata.Name)