fix(pipelineTemplateSave): Take tags from template files (#355)

* fix(pipelineTemplateSave): Take tags from template files

Spin ignores tags from pipeline template files, and it causes errors during saving.
Example of the problem:
1. Create a template with a tag "stable" inside
2. Save it via `spin pt save`. Don't pass the tag to the command, because it already exists in the code
3. Front50 parses the template and finds the tag inside: https://github.com/spinnaker/front50/blob/master/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/V2PipelineTemplateController.java#L94
4. Front50 saves the template with the tag
5. Edit the code of the template and save it again
6. Spin tries to find a template in Front50 without tags (because we don't pass the tag via arguments)
7. Spin can't find the template and invokes "create" method instead of "update"
8. Front50 throws an DuplicateEntityException because a template with the id already exists. Validation code: https://github.com/spinnaker/front50/blob/master/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/V2PipelineTemplateController.java#L115

Front50 takes a tag from query parameters as a priority. And only if the tag is not in the query parameters, it tries to take it from the template source code. https://github.com/spinnaker/front50/blob/master/front50-web/src/main/java/com/netflix/spinnaker/front50/controllers/V2PipelineTemplateController.java#L93
I reproduced a similar logic in Spin

* fix comment in tests

cmd/pipeline-template/save.go

@@ -84,6 +84,8 @@ func savePipelineTemplate(cmd *cobra.Command, options *saveOptions) error {
 	getQueryParam := &gate.V2PipelineTemplatesControllerApiGetUsingGET2Opts{}
 	if options.tag != "" {
 		getQueryParam.Tag = optional.NewString(options.tag)
+	} else if tag, exists := templateJson["tag"]; exists && tag.(string) != "" {
+		getQueryParam.Tag = optional.NewString(tag.(string))
 	}
 
 	_, resp, queryErr := options.GateClient.V2PipelineTemplatesControllerApi.GetUsingGET2(options.GateClient.Context, templateId, getQueryParam)

cmd/pipeline-template/save_test.go

@@ -133,6 +133,70 @@ func TestPipelineTemplateSave_update(t *testing.T) {
 	util.TestPrettyJsonDiff(t, "save request body", expected, recieved)
 }
 
+func TestPipelineTemplateSave_updatetagfromfile(t *testing.T) {
+	saveBuffer := new(bytes.Buffer)
+	method := new(string)
+	ts := testGatePipelineTemplateUpdateTagSuccess(saveBuffer, method, "stable")
+	defer ts.Close()
+
+	tempFile := tempPipelineTemplateFile(testPipelineTemplateWithTagJsonStr)
+	if tempFile == nil {
+		t.Fatal("Could not create temp pipeline template file.")
+	}
+	defer os.Remove(tempFile.Name())
+
+	rootCmd, rootOpts := cmd.NewCmdRoot(ioutil.Discard, ioutil.Discard)
+	rootCmd.AddCommand(NewPipelineTemplateCmd(rootOpts))
+
+	args := []string{"pipeline-template", "save", "--file", tempFile.Name(), "--gate-endpoint", ts.URL}
+	rootCmd.SetArgs(args)
+	err := rootCmd.Execute()
+	if err != nil {
+		t.Fatalf("Command failed with: %s", err)
+	}
+
+	expected := strings.TrimSpace(testPipelineTemplateWithTagJsonStr)
+	recieved := saveBuffer.Bytes()
+	util.TestPrettyJsonDiff(t, "save request body", expected, recieved)
+
+	// Verify that the commad used the tag from the file
+	if *method != "update" {
+		t.Fatalf("Expected 'update' request, got %s", *method)
+	}
+}
+
+func TestPipelineTemplateSave_taginargumentsandfile(t *testing.T) {
+	saveBuffer := new(bytes.Buffer)
+	method := new(string)
+	ts := testGatePipelineTemplateUpdateTagSuccess(saveBuffer, method, "test")
+	defer ts.Close()
+
+	tempFile := tempPipelineTemplateFile(testPipelineTemplateWithTagJsonStr)
+	if tempFile == nil {
+		t.Fatal("Could not create temp pipeline template file.")
+	}
+	defer os.Remove(tempFile.Name())
+
+	rootCmd, rootOpts := cmd.NewCmdRoot(ioutil.Discard, ioutil.Discard)
+	rootCmd.AddCommand(NewPipelineTemplateCmd(rootOpts))
+
+	args := []string{"pipeline-template", "save", "--file", tempFile.Name(), "--tag", "test", "--gate-endpoint", ts.URL}
+	rootCmd.SetArgs(args)
+	err := rootCmd.Execute()
+	if err != nil {
+		t.Fatalf("Command failed with: %s", err)
+	}
+
+	expected := strings.TrimSpace(testPipelineTemplateWithTagJsonStr)
+	recieved := saveBuffer.Bytes()
+	util.TestPrettyJsonDiff(t, "save request body", expected, recieved)
+
+	// Tag in arguments should take precedence over tag in file
+	if *method != "update" {
+		t.Fatalf("Expected 'update' request, got %s", *method)
+	}
+}
+
 func TestPipelineTemplateSave_updatetag(t *testing.T) {
 	saveBuffer := new(bytes.Buffer)
 	ts := testGatePipelineTemplateUpdateSuccess(saveBuffer)
@@ -301,6 +365,39 @@ func tempPipelineTemplateFile(pipelineContent string) *os.File {
 	return tempFile
 }
 
+// testGatePipelineTemplateUpdateTagSuccess spins up a local http server that we will configure the GateClient
+// to direct requests to.
+// Responds with OK to indicate a pipeline template with an expected tag exists.
+// Responds with 404 NotFound to indicate a pipeline template with an expected tag doesn't exist.
+// Accepts POST calls for create and update requests.
+// Writes used method and request body to buffer for testing.
+func testGatePipelineTemplateUpdateTagSuccess(buffer io.Writer, method *string, expectedTag string) *httptest.Server {
+	mux := util.TestGateMuxWithVersionHandler()
+	mux.Handle(
+		"/v2/pipelineTemplates/update/testSpelTemplate",
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			*method = "update"
+			util.NewTestBufferHandlerFunc(http.MethodPost, buffer, http.StatusOK, "").ServeHTTP(w, r)
+		}),
+	)
+	mux.Handle(
+		"/v2/pipelineTemplates/create",
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			*method = "create"
+			util.NewTestBufferHandlerFunc(http.MethodPost, buffer, http.StatusAccepted, "").ServeHTTP(w, r)
+		}),
+	)
+	// Return that we found an MPT if a tag from the request equals to expectedTag.
+	mux.Handle("/v2/pipelineTemplates/testSpelTemplate", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Query().Get("tag") == expectedTag {
+			w.WriteHeader(http.StatusOK)
+		} else {
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	return httptest.NewServer(mux)
+}
+
 // testGatePipelineTemplateUpdateSuccess spins up a local http server that we will configure the GateClient
 // to direct requests to. Responds with OK to indicate a pipeline template exists,
 // and Accepts POST calls.
@@ -501,6 +598,63 @@ const testPipelineTemplateJsonStr = `
 }
 `
 
+const testPipelineTemplateWithTagJsonStr = `
+{
+ "id": "testSpelTemplate",
+ "lastModifiedBy": "anonymous",
+ "metadata": {
+  "description": "A generic application bake and tag pipeline.",
+  "name": "Default Bake and Tag",
+  "owner": "[email protected]",
+  "scopes": [
+   "global"
+  ]
+ },
+ "pipeline": {
+  "description": "",
+  "keepWaitingPipelines": false,
+  "lastModifiedBy": "anonymous",
+  "limitConcurrent": true,
+  "notifications": [],
+  "parameterConfig": [],
+  "stages": [
+   {
+    "name": "My Wait Stage",
+    "refId": "wait1",
+    "requisiteStageRefIds": [],
+    "type": "wait",
+    "waitTime": "${ templateVariables.waitTime }"
+   }
+  ],
+  "triggers": [
+   {
+    "attributeConstraints": {},
+    "enabled": true,
+    "payloadConstraints": {},
+    "pubsubSystem": "google",
+    "source": "jake",
+    "subscription": "super-why",
+    "subscriptionName": "super-why",
+    "type": "pubsub"
+   }
+  ],
+  "updateTs": "1543509523663"
+ },
+ "protect": false,
+ "schema": "v2",
+ "tag": "stable",
+ "updateTs": "1544475186050",
+ "variables": [
+  {
+   "defaultValue": 42,
+   "description": "The time a wait stage shall pauseth",
+   "name": "waitTime",
+   "type": "int"
+  }
+ ]
+}
+`
+
 const testPipelineTemplateYamlStr = `
 id: testSpelTemplate
 lastModifiedBy: anonymous