From 0c57b08eebe6786d4486f63b34f39e74c77510bc Mon Sep 17 00:00:00 2001 From: kargakis Date: Tue, 10 Nov 2015 11:43:55 +0100 Subject: [PATCH] oc: Make env use PATCH instead of PUT --- hack/cmd_util.sh | 2 +- pkg/cmd/cli/cmd/env.go | 64 +++++++++++++++---- test/cmd/deployments.sh | 32 +++++++++- .../fixtures/test-deployment-config.json | 6 -- 4 files changed, 83 insertions(+), 21 deletions(-) diff --git a/hack/cmd_util.sh b/hack/cmd_util.sh index b82fb1cba8fa..b6cf67410ffd 100644 --- a/hack/cmd_util.sh +++ b/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 diff --git a/pkg/cmd/cli/cmd/env.go b/pkg/cmd/cli/cmd/env.go index 42dd2c2dbab0..279aca874edc 100644 --- a/pkg/cmd/cli/cmd/env.go +++ b/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 diff --git a/test/cmd/deployments.sh b/test/cmd/deployments.sh index 2a9a34c68373..580a3e91dd9e 100755 --- a/test/cmd/deployments.sh +++ b/test/cmd/deployments.sh @@ -9,7 +9,7 @@ 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' @@ -17,8 +17,16 @@ os::cmd::expect_success 'oc create -f test/integration/fixtures/test-deployment- 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' diff --git a/test/integration/fixtures/test-deployment-config.json b/test/integration/fixtures/test-deployment-config.json index 0a4802308864..2da88bbdcefc 100644 --- a/test/integration/fixtures/test-deployment-config.json +++ b/test/integration/fixtures/test-deployment-config.json @@ -38,12 +38,6 @@ "protocol": "TCP" } ], - "env": [ - { - "name": "TEST", - "value": "value" - } - ], "resources": { "limits": { "cpu": "100m",