Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

oc: Make env use PATCH instead of PUT #5819

Merged
merged 1 commit into from
Dec 2, 2015
Merged

oc: Make env use PATCH instead of PUT #5819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hack/cmd_util.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
64 changes: 53 additions & 11 deletions pkg/cmd/cli/cmd/env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cmd

import (
"bufio"
"encoding/json"
"fmt"
"io"
"os"
Expand All @@ -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"
)
Expand Down Expand Up @@ -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.")
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double check the length of the oldObjects you got back... looks like AsVersionedObjects silently skips infos without an Object set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. Any better suggestions for the error message than the current?

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 {
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only place outputVersion should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, check the length

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

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])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have good tests for these cases, to make sure patching is correctly handling:

  • adding env var to a nil list
  • adding env var to an empty list
  • adding env var to a list of existing env vars
  • updating single env var
  • updating multiple env vars (at the beginning, middle, and end of a list)
  • removing env var from a list containing only that env var
  • removing env var from the beginning, middle, and end of a list containing other envvars
  • simultaneously adding, updating and removing three different envvars

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
Expand Down
32 changes: 29 additions & 3 deletions test/cmd/deployments.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevekuznetsov would be handy to be able to check multiple things against the output of running a command a single time, but os::cmd::internal::expect_exit_code_run_grep isn't really factored to allow that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean similar to "-e 'text' -e 'text'" ? I think that should work, but I don't know if that requires every test to succeed or just one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #6091

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, would like a way to check that multiple things exist in the output without caring about order (or really, about grep flags)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for ignoring grep

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On an aside, why are we setting seven envars here? Would one do? If we want to ensure we can parse multiple, will two do?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevekuznetsov #5819 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. A similar result could probably be had by doing one at a time, the only thing that depends on more than one being present was the out-of-order bit. @Kargakis could you paste sample output from oc env dc/test-deployment-config --list ? Maybe there's a smart regex to do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'((A=a|B=b|C=c|D=d|E=e|F=f|G=g).*){7}' is a hard-to-understand way to do this... I'd rather have the individual checks... I want these tests to be very inspectable

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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for multiple negative matches

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess multiple negative matches could be combined to A=a|C=c|G=g already

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'
Expand Down
6 changes: 0 additions & 6 deletions test/integration/fixtures/test-deployment-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,6 @@
"protocol": "TCP"
}
],
"env": [
{
"name": "TEST",
"value": "value"
}
],
"resources": {
"limits": {
"cpu": "100m",
Expand Down