Merge pull request #5819 from kargakis/use-patch-in-oc-env

Merged by openshift-bot

hack/cmd_util.sh

@@ -141,7 +141,7 @@ os_cmd_internal_tmpdir="/tmp/openshift/test/cmd"
 os_cmd_internal_tmpout="${os_cmd_internal_tmpdir}/tmp_stdout.log"
 os_cmd_internal_tmperr="${os_cmd_internal_tmpdir}/tmp_stderr.log"
 
-# os::cmd::internal::expect_exit_code_and_text runs the provided test command and expects a specific 
+# os::cmd::internal::expect_exit_code_run_grep runs the provided test command and expects a specific 
 # exit code from that command as well as the success of a specified `grep` invocation. Output from the 
 # command to be tested is suppressed unless either `VERBOSE=1` or the test fails. This function bypasses
 # any error exiting settings or traps set by upstream callers by masking the return code of the command 

pkg/cmd/cli/cmd/env.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"bufio"
+	"encoding/json"
 	"fmt"
 	"io"
 	"os"
@@ -13,6 +14,7 @@ import (
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	"k8s.io/kubernetes/pkg/kubectl/resource"
 	"k8s.io/kubernetes/pkg/util/sets"
+	"k8s.io/kubernetes/pkg/util/strategicpatch"
 
 	"github.com/openshift/origin/pkg/cmd/util/clientcmd"
 )
@@ -77,7 +79,7 @@ func NewCmdEnv(fullName string, f *clientcmd.Factory, in io.Reader, out io.Write
 	cmd.Flags().StringSliceVarP(&env, "env", "e", env, "Specify key value pairs of environment variables to set into each container.")
 	cmd.Flags().Bool("list", false, "Display the environment and any changes in the standard format")
 	cmd.Flags().StringP("selector", "l", "", "Selector (label query) to filter on")
-	cmd.Flags().Bool("all", false, "select all resources in the namespace of the specified resource types")
+	cmd.Flags().Bool("all", false, "Select all resources in the namespace of the specified resource types")
 	cmd.Flags().StringSliceVarP(&filenames, "filename", "f", filenames, "Filename, directory, or URL to file to use to edit the resource.")
 	cmd.Flags().Bool("overwrite", true, "If true, allow environment to be overwritten, otherwise reject updates that overwrite existing environment.")
 	cmd.Flags().String("resource-version", "", "If non-empty, the labels update will only succeed if this is the current resource-version for the object. Only valid when specifying a single resource.")
@@ -201,7 +203,6 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman
 	if err != nil {
 		return err
 	}
-	outputVersion := cmdutil.OutputVersion(cmd, clientConfig.Version)
 
 	cmdNamespace, explicit, err := f.DefaultNamespace()
 	if err != nil {
@@ -232,6 +233,23 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman
 	if !one && len(resourceVersion) > 0 {
 		return cmdutil.UsageError(cmd, "--resource-version may only be used with a single resource")
 	}
+	// Keep a copy of the original objects prior to updating their environment.
+	// Used in constructing the patch(es) that will be applied in the server.
+	oldObjects, err := resource.AsVersionedObjects(infos, clientConfig.Version)
+	if err != nil {
+		return err
+	}
+	if len(oldObjects) != len(infos) {
+		return fmt.Errorf("could not convert all objects to API version %q", clientConfig.Version)
+	}
+	oldData := make([][]byte, len(infos))
+	for i := range oldObjects {
+		old, err := json.Marshal(oldObjects[i])
+		if err != nil {
+			return err
+		}
+		oldData[i] = old
+	}
 
 	skipped := 0
 	for _, info := range infos {
@@ -268,29 +286,53 @@ func RunEnv(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra.Comman
 		}
 	}
 	if one && skipped == len(infos) {
-		return fmt.Errorf("the %s %s is not a pod or does not have a pod template", infos[0].Mapping.Resource, infos[0].Name)
+		return fmt.Errorf("%s/%s is not a pod or does not have a pod template", infos[0].Mapping.Resource, infos[0].Name)
 	}
 
 	if list {
 		return nil
 	}
 
-	objects, err := resource.AsVersionedObject(infos, false, outputVersion)
-	if err != nil {
-		return err
-	}
-
 	if len(outputFormat) != 0 {
+		outputVersion := cmdutil.OutputVersion(cmd, clientConfig.Version)
+		objects, err := resource.AsVersionedObjects(infos, outputVersion)
+		if err != nil {
+			return err
+		}
+		if len(objects) != len(infos) {
+			return fmt.Errorf("could not convert all objects to API version %q", outputVersion)
+		}
 		p, _, err := kubectl.GetPrinter(outputFormat, "")
 		if err != nil {
 			return err
 		}
-		return p.PrintObj(objects, out)
+		for _, object := range objects {
+			if err := p.PrintObj(object, out); err != nil {
+				return err
+			}
+		}
+		return nil
+	}
+
+	objects, err := resource.AsVersionedObjects(infos, clientConfig.Version)
+	if err != nil {
+		return err
+	}
+	if len(objects) != len(infos) {
+		return fmt.Errorf("could not convert all objects to API version %q", clientConfig.Version)
 	}
 
 	failed := false
-	for _, info := range infos {
-		obj, err := resource.NewHelper(info.Client, info.Mapping).Replace(info.Namespace, info.Name, true, info.Object)
+	for i, info := range infos {
+		newData, err := json.Marshal(objects[i])
+		if err != nil {
+			return err
+		}
+		patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData[i], newData, objects[i])
+		if err != nil {
+			return err
+		}
+		obj, err := resource.NewHelper(info.Client, info.Mapping).Patch(info.Namespace, info.Name, kapi.StrategicMergePatchType, patchBytes)
 		if err != nil {
 			handlePodUpdateError(cmd.Out(), err, "environment variables")
 			failed = true

test/cmd/deployments.sh

@@ -9,16 +9,24 @@ source "${OS_ROOT}/hack/util.sh"
 source "${OS_ROOT}/hack/cmd_util.sh"
 os::log::install_errexit
 
-# This test validates deployments
+# This test validates deployments and the env command
 
 os::cmd::expect_success 'oc get deploymentConfigs'
 os::cmd::expect_success 'oc get dc'
 os::cmd::expect_success 'oc create -f test/integration/fixtures/test-deployment-config.json'
 os::cmd::expect_success 'oc describe deploymentConfigs test-deployment-config'
 os::cmd::expect_success_and_text 'oc get dc -o name' 'deploymentconfig/test-deployment-config'
 os::cmd::expect_success_and_text 'oc describe dc test-deployment-config' 'deploymentconfig=test-deployment-config'
+
+# Patch a nil list
+os::cmd::expect_success 'oc env dc/test-deployment-config TEST=value'
 os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'TEST=value'
-os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config TEST- --list' 'TEST=value'
+# Remove only env in the list
+os::cmd::expect_success 'oc env dc/test-deployment-config TEST-'
+os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config --list' 'TEST=value'
+# Add back to empty list
+os::cmd::expect_success 'oc env dc/test-deployment-config TEST=value'
+os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config TEST=foo --list' 'TEST=value'
 os::cmd::expect_success_and_text 'oc env dc/test-deployment-config TEST=foo --list' 'TEST=foo'
 os::cmd::expect_success_and_text 'oc env dc/test-deployment-config OTHER=foo --list' 'TEST=value'
 os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config OTHER=foo -c ruby --list' 'OTHER=foo'
@@ -30,7 +38,25 @@ os::cmd::expect_success_and_text 'oc env dc/test-deployment-config OTHER=foo -o
 os::cmd::expect_success_and_text 'echo OTHER=foo | oc env dc/test-deployment-config -e - --list' 'OTHER=foo'
 os::cmd::expect_success_and_not_text 'echo #OTHER=foo | oc env dc/test-deployment-config -e - --list' 'OTHER=foo'
 os::cmd::expect_success 'oc env dc/test-deployment-config TEST=bar OTHER=baz BAR-'
-
+os::cmd::expect_success_and_not_text 'oc env -f test/integration/fixtures/test-deployment-config.json TEST=VERSION -o yaml' 'v1beta3'
+os::cmd::expect_success 'oc env dc/test-deployment-config A=a B=b C=c D=d E=e F=f G=g'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'A=a'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'B=b'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'C=c'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'D=d'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'E=e'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'F=f'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'G=g'
+os::cmd::expect_success 'oc env dc/test-deployment-config H=h G- E=updated C- A-'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'B=b'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'D=d'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'E=updated'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'F=f'
+os::cmd::expect_success_and_text 'oc env dc/test-deployment-config --list' 'H=h'
+os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config --list' 'A=a'
+os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config --list' 'C=c'
+os::cmd::expect_success_and_not_text 'oc env dc/test-deployment-config --list' 'G=g'
+echo "env: ok"
 os::cmd::expect_success 'oc deploy test-deployment-config'
 os::cmd::expect_success 'oc deploy dc/test-deployment-config'
 os::cmd::expect_success 'oc delete deploymentConfigs test-deployment-config'

test/integration/fixtures/test-deployment-config.json

@@ -38,12 +38,6 @@
                 "protocol": "TCP"
               }
             ],
-            "env": [
-              {
-                "name": "TEST",
-                "value": "value"
-              }
-            ],
             "resources": {
               "limits": {
                 "cpu": "100m",