Update set commands #20139

soltysh · 2018-06-28T20:17:47Z

juanvallejo · 2018-06-29T01:53:38Z

pkg/oc/cli/cmd/set/buildsecret.go

-}
+		actual, err := o.Client.Resource(info.Mapping.Resource).Namespace(info.Namespace).Patch(info.Name, types.StrategicMergePatchType, patch.Patch)
+		if err != nil {
+			allErrs = append(allErrs, fmt.Errorf("fialed to patch secret  %v", err))


s/fialed/failed/

juanvallejo · 2018-07-02T13:43:26Z

pkg/oc/cli/cmd/set/buildhook.go

 	"k8s.io/client-go/dynamic"
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"
+	"k8s.io/kubernetes/pkg/printers"


I thought we had switched to using the new pkg/genericclioptions/printers pkg?

juanvallejo · 2018-07-02T13:49:51Z

pkg/oc/cli/cmd/set/buildhook.go

 	for _, patch := range patches {
 		info := patch.Info
+		name := info.Name
+		if info.Mapping != nil {


This fixes the panic we were seeing with --local, right?
(will update upstream PR to match this)

Per @liggitt's comment here, I am wondering if we would would want to do the same in Origin? That is, default to showing lower-case kind.group (with .group being included when non-empty) when info.Mapping is nil?

I've created GetObjectName which I'm guessing you can just add upstream, it does exactly what Jordan asked for.

juanvallejo · 2018-07-02T13:51:17Z

pkg/oc/cli/cmd/set/buildhook.go

 			continue
 		}

 		if string(patch.Patch) == "{}" || len(patch.Patch) == 0 {
-			fmt.Fprintf(o.Err, "info: %s %q was not changed\n", info.Mapping.Resource, info.Name)
+			fmt.Fprintf(o.ErrOut, "info: %s was not changed\n", name)


I thought we were getting rid of this message?

Turned into glog.V(1).Infof("info: %s was not changed\n", name)

juanvallejo · 2018-07-02T13:52:33Z

pkg/oc/cli/cmd/set/buildsecret.go

 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"
+	"k8s.io/kubernetes/pkg/printers"


genericclioptions/printers?

juanvallejo · 2018-07-02T14:05:13Z

pkg/oc/cli/cmd/set/deploymenthook.go

-		return o.PrintObject(infos)
-	}
+	// if singleItemImplied && len(patches) == 0 {
+	// 	return fmt.Errorf("%s/%s is not a deployment config or does not have an applicable strategy", infos[0].Mapping.Resource, infos[0].Name)


Is this leftover, or are we enabling this error message in the future?

Yeah, just fixed.

juanvallejo · 2018-07-02T14:07:54Z

pkg/oc/cli/cmd/set/env.go

-	Client dynamic.Interface
+func NewEnvOptions(streams genericclioptions.IOStreams) *EnvOptions {
+	return &EnvOptions{
+		PrintFlags: genericclioptions.NewPrintFlags("updated"),


.WithTypeSetter(oscheme.PrintingInternalScheme)?

juanvallejo · 2018-07-02T14:22:07Z

pkg/oc/cli/cmd/set/probe.go

@@ -22,6 +21,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"
 	"k8s.io/kubernetes/pkg/kubectl/polymorphichelpers"
+	"k8s.io/kubernetes/pkg/printers"


genericclioptions/printers

juanvallejo · 2018-07-02T14:23:20Z

pkg/oc/cli/cmd/set/probe.go

+	cmd.Flags().StringVar(&o.HTTPGet, "get-url", o.HTTPGet, "A URL to perform an HTTP GET on (you can omit the host, have a string port, or omit the scheme.")
+
+	o.InitialDelaySeconds = cmd.Flags().Int("initial-delay-seconds", 0, "The time in seconds to wait before the probe begins checking")
+	o.SuccessThreshold = cmd.Flags().Int("success-threshold", 0, "The number of successes required before the probe is considered successful")


maybe we could switch these to IntVars?

There are pointer to values, I'm guessing to to differentiate between set and not, although there's .Changed paramter on each flag. But I didn't want to add more to this PR. We can solve it later ;)

soltysh · 2018-07-02T15:29:12Z

Comments addressed, ptal.

soltysh · 2018-07-04T12:35:43Z

I'll wait for #20083 to merge and will tag this one.

openshift-ci-robot · 2018-07-05T08:52:23Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2018-07-05T08:52:58Z

Approving based on previous comments and the included commit from #20083

soltysh · 2018-07-05T12:22:59Z

/retest

soltysh · 2018-07-05T16:20:09Z

/retest

soltysh · 2018-07-05T18:23:32Z

/retest

soltysh · 2018-07-06T08:22:04Z

/retest

soltysh · 2018-07-06T10:31:40Z

/retest

…ted values

openshift-ci-robot · 2018-07-06T11:42:16Z

New changes are detected. LGTM label has been removed.

soltysh · 2018-07-06T11:42:39Z

Rebase only, re-applying the label back.

openshift-ci-robot · 2018-07-06T13:04:28Z

@soltysh: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_builds	`dfc6405`	link	`/test extended_builds`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

soltysh · 2018-07-06T13:16:44Z

/retest

openshift-ci-robot assigned deads2k and juanvallejo Jun 28, 2018

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 28, 2018

openshift-ci-robot requested review from ironcladlou and juanvallejo June 28, 2018 20:17

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 28, 2018

soltysh force-pushed the the_new_oc branch 2 times, most recently from 391e4a0 to dfc6405 Compare June 28, 2018 21:06

juanvallejo reviewed Jun 29, 2018

View reviewed changes

juanvallejo approved these changes Jun 29, 2018

View reviewed changes

soltysh force-pushed the the_new_oc branch from dfc6405 to 67fc372 Compare June 29, 2018 14:55

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2018

soltysh force-pushed the the_new_oc branch 2 times, most recently from 54f426e to df4770c Compare June 29, 2018 16:06

soltysh changed the title ~~WIP: Update set commands~~ Update set commands Jun 29, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2018

soltysh force-pushed the the_new_oc branch 2 times, most recently from 331ae43 to 9c050c0 Compare July 2, 2018 13:27

juanvallejo reviewed Jul 2, 2018

View reviewed changes

soltysh force-pushed the the_new_oc branch from 9c050c0 to 504637b Compare July 2, 2018 15:28

soltysh force-pushed the the_new_oc branch from 504637b to 5fcd6a7 Compare July 3, 2018 16:53

soltysh force-pushed the the_new_oc branch from 255f5f4 to ff9a5cb Compare July 5, 2018 08:49

soltysh mentioned this pull request Jul 5, 2018

add --read-only opt to oc set volumes #20083

Closed

soltysh added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Jul 5, 2018

juanvallejo and others added 7 commits July 6, 2018 13:40

add --read-only opt to oc set volumes

c8ea604

UPSTREAM: 65700: Update output format so that it matches actual accep…

f31bd3a

…ted values

Remove deprecated oc env/volume

64a578d

Update example

0c59375

Update set

2d3c4fb

Switch set triggers to external

d7f4e4d

Generated changes

02b6c04

soltysh force-pushed the the_new_oc branch from ff9a5cb to 02b6c04 Compare July 6, 2018 11:42

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2018

soltysh added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2018

openshift-merge-robot merged commit 00db30d into openshift:master Jul 6, 2018

soltysh deleted the the_new_oc branch July 6, 2018 14:01

This was referenced Aug 9, 2018

Switch to oc set env, since oc env is now removed openshift/openshift-ansible#9497

Merged

Switch to oc set env/volume, since oc env/volume is now removed openshift/openshift-docs#11449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update set commands #20139

Update set commands #20139

soltysh commented Jun 28, 2018

juanvallejo Jun 29, 2018 •

edited

Loading

juanvallejo Jul 2, 2018

juanvallejo Jul 2, 2018 •

edited

Loading

soltysh Jul 2, 2018

juanvallejo Jul 3, 2018

soltysh Jul 4, 2018

juanvallejo Jul 2, 2018

soltysh Jul 2, 2018

juanvallejo Jul 2, 2018

juanvallejo Jul 2, 2018

soltysh Jul 2, 2018

juanvallejo Jul 2, 2018

soltysh Jul 2, 2018

juanvallejo Jul 2, 2018

juanvallejo Jul 2, 2018

soltysh Jul 2, 2018

soltysh commented Jul 2, 2018

soltysh commented Jul 4, 2018

openshift-ci-robot commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 6, 2018

soltysh commented Jul 6, 2018

openshift-ci-robot commented Jul 6, 2018

soltysh commented Jul 6, 2018

openshift-ci-robot commented Jul 6, 2018 •

edited

Loading

soltysh commented Jul 6, 2018

Update set commands #20139

Update set commands #20139

Conversation

soltysh commented Jun 28, 2018

juanvallejo Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Jul 2, 2018

soltysh commented Jul 4, 2018

openshift-ci-robot commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 5, 2018

soltysh commented Jul 6, 2018

soltysh commented Jul 6, 2018

openshift-ci-robot commented Jul 6, 2018

soltysh commented Jul 6, 2018

openshift-ci-robot commented Jul 6, 2018 • edited Loading

soltysh commented Jul 6, 2018

juanvallejo Jun 29, 2018 •

edited

Loading

juanvallejo Jul 2, 2018 •

edited

Loading

openshift-ci-robot commented Jul 6, 2018 •

edited

Loading