feat: added mergo to handle the differences between versions of jx-requirements.yml when upgrading a cluster

go.mod

@@ -67,6 +67,7 @@ require (
 	github.com/hinshun/vt10x v0.0.0-20180809195222-d55458df857c
 	github.com/hpcloud/tail v1.0.0
 	github.com/iancoleman/orderedmap v0.0.0-20181121102841-22c6ecc9fe13
+	github.com/imdario/mergo v0.3.8
 	github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect
 	github.com/jbrukh/bayesian v0.0.0-20161210175230-bf3f261f9a9c // indirect
 	github.com/jenkins-x/draft-repo v0.0.0-20180417100212-2f66cc518135

go.sum

@@ -408,6 +408,8 @@ github.com/imdario/mergo v0.3.6 h1:xTNEAn+kxVO7dTZGu0CegyqKZmoWFI0rF8UxjlB2d28=
 github.com/imdario/mergo v0.3.6/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
 github.com/imdario/mergo v0.3.7 h1:Y+UAYTZ7gDEuOfhxKWy+dvb5dRQ6rJjFSdX2HZY1/gI=
 github.com/imdario/mergo v0.3.7/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
+github.com/imdario/mergo v0.3.8 h1:CGgOkSJeqMRmt0D9XLWExdT4m4F1vd3FV3VPt+0VxkQ=
+github.com/imdario/mergo v0.3.8/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
 github.com/inconshreveable/mousetrap v0.0.0-20141017200713-76626ae9c91c/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM=
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=

pkg/cmd/upgrade/upgrade_boot.go

@@ -84,11 +84,11 @@ func (o *UpgradeBootOptions) Run() error {
 		}
 	}
 
-	reqsVersionStream, err := o.requirementsVersionStream()
+	requirements, requirementsFile, err := config.LoadRequirementsConfig(o.Dir)
 	if err != nil {
-		return errors.Wrap(err, "failed to get requirements version stream")
+		return errors.Wrapf(err, "failed to load requirements config %s", requirementsFile)
 	}
-
+	reqsVersionStream := requirements.VersionStream
 	upgradeVersionSha, err := o.upgradeAvailable(reqsVersionStream.URL, reqsVersionStream.Ref, o.UpgradeVersionStreamRef)
 	if err != nil {
 		return errors.Wrap(err, "failed to get check for available update")
@@ -126,6 +126,22 @@ func (o *UpgradeBootOptions) Run() error {
 		return errors.Wrap(err, "failed to update version stream ref")
 	}
 
+	// load modified requirements so we can merge with the base ones
+	modifiedRequirements, modifiedRequirementsFile, err := config.LoadRequirementsConfig(o.Dir)
+	if err != nil {
+		return errors.Wrapf(err, "failed to load requirements config %s", modifiedRequirementsFile)
+	}
+
+	err = requirements.MergeSave(modifiedRequirements, modifiedRequirementsFile)
+	if err != nil {
+		return errors.Wrap(err, "error merging the modified jx-requirements.yml file with the dev environment's one")
+	}
+
+	err = o.createCommitForRequirements(requirementsFile)
+	if err != nil {
+		return errors.Wrap(err, "failed to create a merge commit for jx-requirements.yml")
+	}
+
 	err = o.raisePR()
 	if err != nil {
 		return errors.Wrap(err, "failed to raise pr")
@@ -138,6 +154,15 @@ func (o *UpgradeBootOptions) Run() error {
 	return nil
 }
 
+func (o UpgradeBootOptions) createCommitForRequirements(requirementsFileName string) error {
+	err := o.Git().AddCommitFiles(o.Dir, "Merge jx-requirements.yml", []string{requirementsFileName})
+	if err != nil {
+		return errors.Wrapf(err, "error creating a commit with the merged jx-requirements.yml file from dir %s",
+			requirementsFileName)
+	}
+	return nil
+}
+
 func (o UpgradeBootOptions) determineBootConfigURL(versionStreamURL string) (string, error) {
 	var bootConfigURL string
 	if versionStreamURL == config.DefaultVersionsURL {
@@ -171,22 +196,6 @@ func (o UpgradeBootOptions) determineBootConfigURL(versionStreamURL string) (str
 	return bootConfigURL, nil
 }
 
-func (o *UpgradeBootOptions) requirementsVersionStream() (*config.VersionStreamConfig, error) {
-	requirements, requirementsFile, err := config.LoadRequirementsConfig(o.Dir)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to load requirements config %s", requirementsFile)
-	}
-	exists, err := util.FileExists(requirementsFile)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to check if file %s exists", requirementsFile)
-	}
-	if !exists {
-		return nil, fmt.Errorf("no requirements file %s ensure you are running this command inside a GitOps clone", requirementsFile)
-	}
-	reqsVersionStream := requirements.VersionStream
-	return &reqsVersionStream, nil
-}
-
 func (o *UpgradeBootOptions) upgradeAvailable(versionStreamURL string, versionStreamRef string, upgradeRef string) (string, error) {
 	versionsDir, _, err := o.CloneJXVersionsRepo(versionStreamURL, upgradeRef)
 	if err != nil {

pkg/cmd/upgrade/upgrade_boot_test.go

@@ -41,8 +41,9 @@ func TestDetermineBootConfigURL(t *testing.T) {
 	o := TestUpgradeBootOptions{}
 	o.setup(defaultBootRequirements)
 
-	vs, err := o.requirementsVersionStream()
-	require.NoError(t, err, "could not get requirements version stream")
+	requirements, _, err := config.LoadRequirementsConfig(o.UpgradeBootOptions.Dir)
+	require.NoError(t, err, "could not get requirements file")
+	vs := &requirements.VersionStream
 
 	URL, err := o.determineBootConfigURL(vs.URL)
 	require.NoError(t, err, "could not determine boot config URL")
@@ -55,8 +56,9 @@ func TestRequirementsVersionStream(t *testing.T) {
 	o := TestUpgradeBootOptions{}
 	o.setup(defaultBootRequirements)
 
-	vs, err := o.requirementsVersionStream()
-	require.NoError(t, err, "could not get requirements version stream")
+	requirements, _, err := config.LoadRequirementsConfig(o.UpgradeBootOptions.Dir)
+	require.NoError(t, err, "could not get requirements file")
+	vs := &requirements.VersionStream
 
 	assert.Equal(t, "2367726d02b8c", vs.Ref, "RequirementsVersionStream Ref")
 	assert.Equal(t, "https://github.com/jenkins-x/jenkins-x-versions.git", vs.URL, "RequirementsVersionStream URL")
@@ -79,8 +81,9 @@ func TestUpdateVersionStreamRef(t *testing.T) {
 	err := o.updateVersionStreamRef("22222222")
 	require.NoError(t, err, "could not update version stream ref")
 
-	vs, err := o.requirementsVersionStream()
-	require.NoError(t, err, "could not get requirements version stream")
+	requirements, _, err := config.LoadRequirementsConfig(o.UpgradeBootOptions.Dir)
+	require.NoError(t, err, "could not get requirements file")
+	vs := &requirements.VersionStream
 	assert.Equal(t, "22222222", vs.Ref, "UpdateVersionStreamRef Ref")
 }
 
@@ -133,8 +136,9 @@ func TestDetermineBootConfigURLAlternative(t *testing.T) {
 	o := TestUpgradeBootOptions{}
 	o.setup(alternativeBootRequirements)
 
-	vs, err := o.requirementsVersionStream()
-	require.NoError(t, err, "could not get requirements version stream")
+	requirements, _, err := config.LoadRequirementsConfig(o.UpgradeBootOptions.Dir)
+	require.NoError(t, err, "could not get requirements file")
+	vs := &requirements.VersionStream
 
 	URL, err := o.determineBootConfigURL(vs.URL)
 	require.NoError(t, err, "could not determine boot config URL")
@@ -147,8 +151,9 @@ func TestRequirementsVersionStreamAlternative(t *testing.T) {
 	o := TestUpgradeBootOptions{}
 	o.setup(alternativeBootRequirements)
 
-	vs, err := o.requirementsVersionStream()
-	require.NoError(t, err, "could not get requirements version stream")
+	requirements, _, err := config.LoadRequirementsConfig(o.UpgradeBootOptions.Dir)
+	require.NoError(t, err, "could not get requirements file")
+	vs := &requirements.VersionStream
 
 	assert.Equal(t, "2367726d02b9c", vs.Ref, "RequirementsVersionStream Ref")
 	assert.Equal(t, "https://github.com/some-org/some-org-jenkins-x-versions.git", vs.URL, "RequirementsVersionStream URL")
@@ -171,7 +176,9 @@ func TestUpdateVersionStreamRefAlternative(t *testing.T) {
 	err := o.updateVersionStreamRef("22222222")
 	require.NoError(t, err, "could not update version stream ref")
 
-	vs, err := o.requirementsVersionStream()
-	require.NoError(t, err, "could not get requirements version stream")
+	requirements, _, err := config.LoadRequirementsConfig(o.UpgradeBootOptions.Dir)
+	require.NoError(t, err, "could not get requirements file")
+	vs := &requirements.VersionStream
+
 	assert.Equal(t, "22222222", vs.Ref, "UpdateVersionStreamRef Ref")
 }

pkg/config/install_requirements.go

@@ -6,6 +6,8 @@ import (
 	"os"
 	"strings"
 
+	"github.com/imdario/mergo"
+
 	"github.com/ghodss/yaml"
 	v1 "github.com/jenkins-x/jx/pkg/apis/jenkins.io/v1"
 	"github.com/jenkins-x/jx/pkg/cloud"
@@ -536,6 +538,51 @@ func (c *RequirementsConfig) SaveConfig(fileName string) error {
 	return nil
 }
 
+type environmentsSliceTransformer struct{}
+
+// environmentsSliceTransformer.Transformer is handling the correct merge of two EnvironmentConfig slices
+// so we can both append extra items and merge existing ones so we don't lose any data
+func (t environmentsSliceTransformer) Transformer(typ reflect.Type) func(dst, src reflect.Value) error {
+	if typ == reflect.TypeOf([]EnvironmentConfig{}) {
+		return func(dst, src reflect.Value) error {
+			d := dst.Interface().([]EnvironmentConfig)
+			s := src.Interface().([]EnvironmentConfig)
+			if dst.CanSet() {
+				for i, v := range s {
+					if i > len(d)-1 {
+						d = append(d, v)
+					} else {
+						err := mergo.Merge(&d[i], &v, mergo.WithOverride)
+						if err != nil {
+							return errors.Wrap(err, "error merging EnvironmentConfig slices")
+						}
+					}
+				}
+				dst.Set(reflect.ValueOf(d))
+			}
+			return nil
+		}
+	}
+	return nil
+}
+
+// MergeSave attempts to merge the provided RequirementsConfig with the caller's data.
+// It does so overriding values in the source struct with non-zero values from the provided struct
+// it defines non-zero per property and not for a while embedded struct, meaning that nested properties
+// in embedded structs should also be merged correctly.
+// if a slice is added a transformer will be needed to handle correctly merging the contained values
+func (c *RequirementsConfig) MergeSave(src *RequirementsConfig, requirementsFileName string) error {
+	err := mergo.Merge(c, src, mergo.WithOverride, mergo.WithTransformers(environmentsSliceTransformer{}))
+	if err != nil {
+		return errors.Wrap(err, "error merging jx-requirements.yml files")
+	}
+	err = c.SaveConfig(requirementsFileName)
+	if err != nil {
+		return errors.Wrapf(err, "error saving the merged jx-requirements.yml files to %s", requirementsFileName)
+	}
+	return nil
+}
+
 // EnvironmentMap creates a map of maps tree which can be used inside Go templates to access the environment
 // configurations
 func (c *RequirementsConfig) EnvironmentMap() map[string]interface{} {

pkg/config/install_requirements_test.go

@@ -7,9 +7,10 @@ import (
 	"testing"
 
 	"github.com/ghodss/yaml"
-	"github.com/jenkins-x/jx/pkg/log"
-
+	"github.com/jenkins-x/jx/pkg/cloud"
 	"github.com/jenkins-x/jx/pkg/config"
+	"github.com/jenkins-x/jx/pkg/gits"
+	"github.com/jenkins-x/jx/pkg/log"
 	"github.com/jenkins-x/jx/pkg/util"
 	"github.com/stretchr/testify/assert"
 )
@@ -141,6 +142,159 @@ func Test_env_repository_visibility(t *testing.T) {
 	}
 }
 
+func TestMergeSave(t *testing.T) {
+	t.Parallel()
+	type TestSpec struct {
+		Name           string
+		Original       *config.RequirementsConfig
+		Changed        *config.RequirementsConfig
+		ValidationFunc func(orig *config.RequirementsConfig, ch *config.RequirementsConfig)
+	}
+
+	testCases := []TestSpec{
+		{
+			Name: "Merge Cluster Config Test",
+			Original: &config.RequirementsConfig{
+				Cluster: config.ClusterConfig{
+					EnvironmentGitOwner:  "owner",
+					EnvironmentGitPublic: false,
+					GitPublic:            false,
+					Provider:             cloud.GKE,
+					Namespace:            "jx",
+					ProjectID:            "project-id",
+					ClusterName:          "cluster-name",
+					Region:               "region",
+					GitKind:              gits.KindGitHub,
+					GitServer:            gits.KindGitHub,
+				},
+			},
+			Changed: &config.RequirementsConfig{
+				Cluster: config.ClusterConfig{
+					EnvironmentGitPublic: true,
+					GitPublic:            true,
+					Provider:             cloud.GKE,
+				},
+			},
+			ValidationFunc: func(orig *config.RequirementsConfig, ch *config.RequirementsConfig) {
+				assert.True(t, orig.Cluster.EnvironmentGitPublic == ch.Cluster.EnvironmentGitPublic &&
+					orig.Cluster.GitPublic == ch.Cluster.GitPublic &&
+					orig.Cluster.ProjectID != ch.Cluster.ProjectID, "ClusterConfig validation failed")
+			},
+		},
+		{
+			Name: "Merge EnvironmentConfig slices Test",
+			Original: &config.RequirementsConfig{
+				Environments: []config.EnvironmentConfig{
+					{
+						Key:        "dev",
+						Repository: "repo",
+					},
+					{
+						Key: "production",
+						Ingress: config.IngressConfig{
+							Domain: "domain",
+						},
+					},
+					{
+						Key: "staging",
+						Ingress: config.IngressConfig{
+							Domain: "domainStaging",
+							TLS: config.TLSConfig{
+								Email: "email",
+							},
+						},
+					},
+				},
+			},
+			Changed: &config.RequirementsConfig{
+				Environments: []config.EnvironmentConfig{
+					{
+						Key:   "dev",
+						Owner: "owner",
+					},
+					{
+						Key: "production",
+						Ingress: config.IngressConfig{
+							CloudDNSSecretName: "secret",
+						},
+					},
+					{
+						Key: "staging",
+						Ingress: config.IngressConfig{
+							Domain:          "newDomain",
+							DomainIssuerURL: "issuer",
+							TLS: config.TLSConfig{
+								Enabled: true,
+							},
+						},
+					},
+					{
+						Key: "ns2",
+					},
+				},
+			},
+			ValidationFunc: func(orig *config.RequirementsConfig, ch *config.RequirementsConfig) {
+				assert.True(t, len(orig.Environments) == len(ch.Environments), "the environment slices should be of the same len")
+				// -- Dev Environment's asserts
+				devOrig, devCh := orig.Environments[0], ch.Environments[0]
+				assert.True(t, devOrig.Owner == devCh.Owner && devOrig.Repository != devCh.Repository,
+					"the dev environment should've been merged correctly")
+				// -- Production Environment's asserts
+				prOrig, prCh := orig.Environments[1], ch.Environments[1]
+				assert.True(t, prOrig.Ingress.Domain == "domain" &&
+					prOrig.Ingress.CloudDNSSecretName == prCh.Ingress.CloudDNSSecretName,
+					"the production environment should've been merged correctly")
+				// -- Staging Environmnet's asserts
+				stgOrig, stgCh := orig.Environments[2], ch.Environments[2]
+				assert.True(t, stgOrig.Ingress.Domain == stgCh.Ingress.Domain &&
+					stgOrig.Ingress.TLS.Email == "email" && stgOrig.Ingress.TLS.Enabled == stgCh.Ingress.TLS.Enabled,
+					"the staging environment should've been merged correctly")
+			},
+		},
+		{
+			Name: "Merge StorageConfig test",
+			Original: &config.RequirementsConfig{
+				Storage: config.StorageConfig{
+					Logs: config.StorageEntryConfig{
+						Enabled: true,
+						URL:     "value1",
+					},
+					Reports: config.StorageEntryConfig{},
+					Repository: config.StorageEntryConfig{
+						Enabled: true,
+						URL:     "value3",
+					},
+				},
+			},
+			Changed: &config.RequirementsConfig{
+				Storage: config.StorageConfig{
+					Reports: config.StorageEntryConfig{
+						Enabled: true,
+						URL:     "",
+					},
+				},
+			},
+			ValidationFunc: func(orig *config.RequirementsConfig, ch *config.RequirementsConfig) {
+				assert.True(t, orig.Storage.Logs.Enabled && orig.Storage.Logs.URL == "value1" &&
+					orig.Storage.Repository.Enabled && orig.Storage.Repository.URL == "value3" &&
+					orig.Storage.Reports.Enabled == ch.Storage.Reports.Enabled,
+					"The storage configuration should've been merged correctly")
+			},
+		},
+	}
+	f, err := ioutil.TempFile("", "")
+	assert.NoError(t, err)
+	defer util.DeleteFile(f.Name())
+
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			err = tc.Original.MergeSave(tc.Changed, f.Name())
+			assert.NoError(t, err, "the merge shouldn't fail for case %s", tc.Name)
+			tc.ValidationFunc(tc.Original, tc.Changed)
+		})
+	}
+}
+
 func Test_EnvironmentGitPublic_and_EnvironmentGitPrivate_specified_together_return_error(t *testing.T) {
 	content, err := ioutil.ReadFile(path.Join(testDataDir, "git_public_true_git_private_true.yaml"))
 	assert.NoError(t, err)