Fix: spec attribute propagation from original_source_yaml (#113)

## What

* Fix spec attribute propagation from `original_source_yaml`
* Misc: fix error handling in spec attribute propagation

## Why

* When defining a Pipeline spec via original_yaml_string, it will fail
to run. See #112

## Notes
<!-- Add any notes here -->

Closes
#112

## Checklist

* [x] _I have read
[CONTRIBUTING.md](https://github.com/codefresh-io/terraform-provider-codefresh/blob/master/README.md)._
* [x] _I have [allowed changes to my fork to be
made](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork)._
* [x] _I have added tests, assuming new tests are warranted_.
* [x] _I understand that the `/test` comment will be ignored by the CI
trigger [unless it is made by a repo admin or
collaborator](https://codefresh.io/docs/docs/pipelines/triggers/git-triggers/#support-for-building-pull-requests-from-forks)._

Author: korenyoni

codefresh.yml

@@ -15,14 +15,14 @@ steps:
   go_fmt:
     title: "Formatting"
     stage: test
-    image: goreleaser/goreleaser:v1.15.2
+    image: goreleaser/goreleaser:v1.17.0
     commands:
       - go fmt
 
   go_test:
     title: "Run tests"
     stage: test
-    image: goreleaser/goreleaser:v1.15.2
+    image: goreleaser/goreleaser:v1.17.0
     environment:
       - TF_ACC="test"
     commands:
@@ -52,7 +52,7 @@ steps:
 
   release_binaries:
     title: Create release in Github
-    image: goreleaser/goreleaser:v1.15.2
+    image: goreleaser/goreleaser:v1.17.0
     stage: release
     environment:
       - GPG_FINGERPRINT=${{GPG_FINGERPRINT}}

codefresh/resource_pipeline.go

@@ -1,18 +1,17 @@
 package codefresh
 
 import (
-	"encoding/json"
 	"fmt"
 	"log"
 	"regexp"
+	"strconv"
 	"strings"
 
 	cfClient "github.com/codefresh-io/terraform-provider-codefresh/client"
 	"github.com/hashicorp/go-cty/cty"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
-	"gopkg.in/yaml.v2"
 )
 
 var terminationPolicyOnCreateBranchAttributes = []string{"branchName", "ignoreTrigger", "ignoreBranch"}
@@ -482,9 +481,12 @@ func resourcePipelineCreate(d *schema.ResourceData, meta interface{}) error {
 
 	client := meta.(*cfClient.Client)
 
-	pipeline := *mapResourceToPipeline(d)
+	pipeline, err := mapResourceToPipeline(d)
+	if err != nil {
+		return err
+	}
 
-	resp, err := client.CreatePipeline(&pipeline)
+	resp, err := client.CreatePipeline(pipeline)
 	if err != nil {
 		return err
 	}
@@ -522,10 +524,14 @@ func resourcePipelineUpdate(d *schema.ResourceData, meta interface{}) error {
 
 	client := meta.(*cfClient.Client)
 
-	pipeline := *mapResourceToPipeline(d)
+	pipeline, err := mapResourceToPipeline(d)
+	if err != nil {
+		return err
+	}
+
 	pipeline.Metadata.ID = d.Id()
 
-	_, err := client.UpdatePipeline(&pipeline)
+	_, err = client.UpdatePipeline(pipeline)
 	if err != nil {
 		return err
 	}
@@ -735,7 +741,7 @@ func flattenTriggers(triggers []cfClient.Trigger) []map[string]interface{} {
 	return res
 }
 
-func mapResourceToPipeline(d *schema.ResourceData) *cfClient.Pipeline {
+func mapResourceToPipeline(d *schema.ResourceData) (*cfClient.Pipeline, error) {
 
 	tags := d.Get("tags").(*schema.Set).List()
 
@@ -774,7 +780,10 @@ func mapResourceToPipeline(d *schema.ResourceData) *cfClient.Pipeline {
 			Context:  d.Get("spec.0.spec_template.0.context").(string),
 		}
 	} else {
-		extractSpecAttributesFromOriginalYamlString(originalYamlString, pipeline)
+		err := extractSpecAttributesFromOriginalYamlString(originalYamlString, pipeline)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	if _, ok := d.GetOk("spec.0.runtime_environment"); ok {
@@ -870,71 +879,61 @@ func mapResourceToPipeline(d *schema.ResourceData) *cfClient.Pipeline {
 
 	pipeline.Spec.TerminationPolicy = codefreshTerminationPolicy
 
-	return pipeline
+	return pipeline, nil
 }
 
-// extractSpecAttributesFromOriginalYamlString extracts the steps and stages from the original yaml string to enable propagation in the `Spec` attribute of the pipeline
-// We cannot leverage on the standard marshal/unmarshal because the steps attribute needs to maintain the order of elements
-// while by default the standard function doesn't do it because in JSON maps are unordered
-func extractSpecAttributesFromOriginalYamlString(originalYamlString string, pipeline *cfClient.Pipeline) {
-	ms := OrderedMapSlice{}
-	err := yaml.Unmarshal([]byte(originalYamlString), &ms)
-	if err != nil {
-		log.Fatalf("Unable to unmarshall original_yaml_string. Error: %v", err)
-	}
+// This function is used to extract the spec attributes from the original_yaml_string attribute.
+// Typically, unmarshalling the YAML string is problematic because the order of the attributes is not preserved.
+// Namely, we care a lot about the order of the steps and stages attributes.
+// Luckily, the yj package introduces a MapSlice type that preserves the order Map items (see utils.go).
+func extractSpecAttributesFromOriginalYamlString(originalYamlString string, pipeline *cfClient.Pipeline) error {
+	for _, attribute := range []string{"stages", "steps", "hooks"} {
+		yamlString, err := yq(fmt.Sprintf(".%s", attribute), originalYamlString)
+		if err != nil {
+			return fmt.Errorf("error while extracting '%s' from original YAML string: %v", attribute, err)
+		} else if yamlString == "" {
+			continue
+		}
 
-	stages := "[]"
-	steps := "{}"
-	hooks := "{}"
+		attributeJson, err := yamlToJson(yamlString)
+		if err != nil {
+			return fmt.Errorf("error while converting '%s' YAML to JSON: %v", attribute, err)
+		}
 
-	// Parse elements of the YAML string to extract Steps, Hooks and Stages if defined
-	for _, item := range ms {
-		key := item.Key.(string)
-		switch key {
+		switch attribute {
+		case "stages":
+			pipeline.Spec.Stages = &cfClient.Stages{
+				Stages: attributeJson,
+			}
 		case "steps":
-			switch x := item.Value.(type) {
-			default:
-				log.Fatalf("unsupported value type: %T", item.Value)
-
-			case OrderedMapSlice:
-				s, _ := json.Marshal(x)
-				steps = string(s)
+			pipeline.Spec.Steps = &cfClient.Steps{
+				Steps: attributeJson,
 			}
-		case "stages":
-			s, _ := json.Marshal(item.Value)
-			stages = string(s)
-
 		case "hooks":
-			switch x := item.Value.(type) {
-			default:
-				log.Fatalf("unsupported value type: %T", item.Value)
-
-			case OrderedMapSlice:
-				h, _ := json.Marshal(x)
-				hooks = string(h)
+			pipeline.Spec.Hooks = &cfClient.Hooks{
+				Hooks: attributeJson,
 			}
-		case "mode":
-			pipeline.Spec.Mode = item.Value.(string)
-		case "fail_fast":
-			ff, ok := item.Value.(bool)
-			if ok {
-				pipeline.Spec.FailFast = &ff
-			}
-		default:
-			log.Printf("Unsupported entry %s", key)
 		}
 	}
 
-	pipeline.Spec.Steps = &cfClient.Steps{
-		Steps: steps,
-	}
-	pipeline.Spec.Stages = &cfClient.Stages{
-		Stages: stages,
-	}
-	pipeline.Spec.Hooks = &cfClient.Hooks{
-		Hooks: hooks,
+	mode, err := yq(".mode", originalYamlString)
+	if err != nil {
+		return fmt.Errorf("error while extracting 'mode' from original YAML string: %v", err)
+	} else if mode != "" {
+		pipeline.Spec.Mode = mode
 	}
 
+	ff, err := yq(".fail_fast", originalYamlString)
+	if err != nil {
+		return fmt.Errorf("error while extracting 'mode' from original YAML string: %v", err)
+	} else if ff != "" {
+		ff_b, err := strconv.ParseBool(strings.TrimSpace(ff))
+		if err != nil {
+			return fmt.Errorf("error while parsing 'fail_fast' as boolean: %v", err)
+		}
+		pipeline.Spec.FailFast = &ff_b
+	}
+	return nil
 }
 
 func getSupportedTerminationPolicyAttributes(policy string) map[string]interface{} {

codefresh/resource_pipeline_test.go

@@ -168,7 +168,59 @@ func TestAccCodefreshPipeline_RuntimeEnvironment(t *testing.T) {
 	})
 }
 
-func TestAccCodefreshPipeline_OriginalYamlString(t *testing.T) {
+func TestAccCodefreshPipeline_OriginalYamlString_Steps(t *testing.T) {
+	name := pipelineNamePrefix + acctest.RandString(10)
+	resourceName := "codefresh_pipeline.test"
+	originalYamlString := `version: 1.0
+steps:
+  cc_firstStep:
+    image: alpine
+    commands:
+      - echo Hello World First Step
+  bb_secondStep:
+    image: alpine
+    commands:
+      - echo Hello World Second jStep
+  aa_secondStep:
+    image: alpine
+    commands:
+      - echo Hello World Third Step`
+
+	expectedSpecAttributes := &cfClient.Spec{
+		Steps: &cfClient.Steps{
+			Steps: `{"cc_firstStep":{"image":"alpine","commands":["echo Hello World First Step"]},"bb_secondStep":{"image":"alpine","commands":["echo Hello World Second jStep"]},"aa_secondStep":{"image":"alpine","commands":["echo Hello World Third Step"]}}`,
+		},
+		Stages: &cfClient.Stages{
+			Stages: `[]`,
+		},
+	}
+
+	var pipeline cfClient.Pipeline
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckCodefreshPipelineDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccCodefreshPipelineBasicConfigOriginalYamlString(name, originalYamlString),
+				Check: resource.ComposeTestCheckFunc(
+
+					testAccCheckCodefreshPipelineExists(resourceName, &pipeline),
+					resource.TestCheckResourceAttr(resourceName, "original_yaml_string", originalYamlString),
+					testAccCheckCodefreshPipelineOriginalYamlStringAttributePropagation(resourceName, expectedSpecAttributes),
+				),
+			},
+			{
+				ResourceName:      resourceName,
+				ImportState:       true,
+				ImportStateVerify: true,
+			},
+		},
+	})
+}
+
+func TestAccCodefreshPipeline_OriginalYamlString_All(t *testing.T) {
 	name := pipelineNamePrefix + acctest.RandString(10)
 	resourceName := "codefresh_pipeline.test"
 	originalYamlString := `version: 1.0

codefresh/utils.go

@@ -1,14 +1,20 @@
 package codefresh
 
 import (
+	"bytes"
 	"fmt"
+	"github.com/sclevine/yj/convert"
 	"log"
 	"regexp"
+	"strings"
 
 	cfClient "github.com/codefresh-io/terraform-provider-codefresh/client"
 	"github.com/dlclark/regexp2"
 	"github.com/ghodss/yaml"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/mikefarah/yq/v4/pkg/yqlib"
+	logging "gopkg.in/op/go-logging.v1"
+	"io/ioutil"
 )
 
 func convertStringArr(ifaceArr []interface{}) []string {
@@ -116,3 +122,42 @@ func stringIsValidRe2RegExp(i interface{}, k string) (warnings []string, errors
 
 	return warnings, errors
 }
+
+// Get a value from a YAML string using yq
+func yq(yamlString string, expression string) (string, error) {
+	yqEncoder := yqlib.NewYamlEncoder(0, false, yqlib.NewDefaultYamlPreferences())
+	yqDecoder := yqlib.NewYamlDecoder(yqlib.NewDefaultYamlPreferences())
+	yqEvaluator := yqlib.NewStringEvaluator()
+
+	// Disable yq logging
+	yqLogBackend := logging.AddModuleLevel(logging.NewLogBackend(ioutil.Discard, "", 0))
+	yqlib.GetLogger().SetBackend(yqLogBackend)
+
+	yamlString, err := yqEvaluator.Evaluate(yamlString, expression, yqEncoder, yqDecoder)
+	yamlString = strings.TrimSpace(yamlString)
+
+	if yamlString == "null" { // yq's Evaluate() returns "null" if the expression does not match anything
+		return "", err
+	}
+	return yamlString, err
+}
+
+// Convert a YAML string to JSON while preserving the order of map keys (courtesy of yj package).
+// If this were to use yaml.Unmarshal() and json.Marshal() instead, the order of map keys would be lost.
+func yamlToJson(yamlString string) (string, error) {
+	yamlConverter := convert.YAML{}
+	jsonConverter := convert.JSON{}
+
+	yamlDecoded, err := yamlConverter.Decode(strings.NewReader(yamlString))
+	if err != nil {
+		return "", err
+	}
+
+	jsonBuffer := new(bytes.Buffer)
+	err = jsonConverter.Encode(jsonBuffer, yamlDecoded)
+	if err != nil {
+		return "", err
+	}
+
+	return jsonBuffer.String(), nil
+}