Simplify rollback arguments

[ironcladlou]

Rolling back a deployment currently requires a reference to a specific deployment/replication controller name. The command should support work in terms of deployment configs and versions, because users shouldn't have to care about specific deployments (which aren't formal API resources).

[ironcladlou]

@smarterclayton @liggitt this needs prioritized.

[smarterclayton]

P2

On Jul 1, 2015, at 11:21 AM, Dan Mace notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton @liggitt https://github.com/liggitt this needs prioritized.

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/3338#issuecomment-117713658.

[ironcladlou]

One thing I thought of while revisiting this: just a number is insufficient because multiple deployment configs are likely. The command signature would need to include options like:

rollback <deployment> (config/version inferred)

or

rollback <config> <version> (deployment inferred)

Would it be too confusing to support both? The 2nd syntax seems like the best one.

[0xmichalis]

This is similar to start-build in that it has to handle dcs and deployments while start-build handles bcs and builds. Should this and https://github.com/openshift/origin/issues/1995 have the same solution @smarterclayton @bparees ?

[bparees]

@kargakis seems reasonable to me.

[deads2k]

I think we should support oc rollback <dc> where it chooses last-known-good-version (if possible) or next-to-last-version by default. That means that the number of args will be insufficient to distinguish these two cases. That takes you into resource builder style names (with a default).

[ironcladlou]

Apparently we should move this conversation to trello. I'm going to close this issue to avoid crosstalk.

Further discussion here: https://trello.com/c/ZREvUIf2

[smarterclayton]

Why are multiple deployment configs likely?

On Jul 8, 2015, at 10:11 AM, Dan Mace notifications@github.com wrote:

One thing I thought of while revisiting this: just a number is insufficient because multiple deployment configs are likely. The command signature would need to include options like:

rollback (config/version inferred)

or

rollback (deployment inferred)

Would it be too confusing to support both? The 2nd syntax seems like the best one.

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/3338#issuecomment-119592681.

[smarterclayton]

A couple of things here. Deploy applies to deployment configs, not to deployments (which aren't an object). The frame of reference is a DC and a version is either an argument (--version 5) or a modification to the name (foo@10, but since we haven't decided how we're doing uid reference I'd like to avoid that for now). So a flag is more appropriate for now.

[smarterclayton]

Why should we move discussion to Trello?

[ironcladlou]

Why should we move discussion to Trello?

@danmcp let's clear this up. We can't have design discussions in two places.

[danmcp]

@ironcladlou I am not sure what we are trying to clear up. Trello is our planning mechanism. It's fine to start in trello and stay in trello. It's fine to create something in trello that references an existing issue in github. It's fine to start in github and finish in github if it's small enough and doesn't require other teams to be involved (QE, Docs, Security, etc). It's fine to start in trello and immediately create a corresponding issue in github where a conversation happens.

[ironcladlou]

I don't think that was made clear at all before. Thanks.

[ironcladlou]

So here's the proposal:

rollback [dc/]name [--version=N]

If --version is omitted, the version of the last completed deployment will be used. If --version is specified, the command will internally find the deployment for that version for the rollback.

[deads2k]

@smarterclayton I think this is another case for having a "default resource" option for resource.Builder.

[smarterclayton]

I don't think we have to support dc/ right now - that's a future thing, and we'd do it upstream first (we're discussing it in a few places).

On Jul 8, 2015, at 1:24 PM, Dan Mace notifications@github.com wrote:

So here's the proposal:

rollback [dc/]name [--version=N]

If --version is omitted, the version of the last completed deployment will be used. If --version is specified, the command will internally find the deployment for that version for the rollback.

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/3338#issuecomment-119669984.

[smarterclayton]

Yeah - possibly as an alternative to ResourceOrType (geared to list or item) we would add Resource (geared to one or more items with a default type). Then, instead of having to support selectors and types on log or deploy, you allow someone to use the new name output format:

oc get dc -s a=b -o name | xargs oc deploy

I don't think we need to support full selection on all items if we have "get -o name"

On Jul 8, 2015, at 1:30 PM, David Eads notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton I think this is another case for having a "default resource" option for resource.Builder.

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/3338#issuecomment-119671375.

[ironcladlou]

Is there any reason not to go ahead and use resource.Builder during this refactor?

[smarterclayton]

Other than forcing dc/foo which I don't like for simplicity. We can two step it if necessary.

On Jul 8, 2015, at 2:33 PM, Dan Mace notifications@github.com wrote:

Is there any reason not to go ahead and use resource.Builder during this refactor?

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin/issues/3338#issuecomment-119691225.

[0xmichalis]

RIght now in the resource builder we can specify a default resource via ResourceTypes but there is no method for accepting just a name of a resource. Opened https://github.com/GoogleCloudPlatform/kubernetes/pull/10979 upstream

[0xmichalis]

I also tested this with oc scale:

        r := resource.NewBuilder(mapper, typer, f.ClientMapperForCommand()).
                ContinueOnError().
                NamespaceParam(cmdNamespace).DefaultNamespace().
-               ResourceTypeOrNameArgs(false, args...).
+               ResourceTypes("ReplicationController").
+               NameParam(args[0]).
                Flatten().
                Do()

Works fine

[vagrant@openshiftdev sample-app]$ oc get rc
CONTROLLER   CONTAINER(S)               IMAGE(S)                            SELECTOR                                                        REPLICAS
database-1   ruby-helloworld-database   openshift/mysql-55-centos7:latest   deployment=database-1,deploymentconfig=database,name=database   0

[vagrant@openshiftdev sample-app]$ oc scale database-1 --replicas=3
scaled

[vagrant@openshiftdev sample-app]$ oc get dc
NAME               TRIGGERS                    LATEST VERSION
database           ConfigChange                1
frontend           ConfigChange, ImageChange   1
ruby-hello-world   ConfigChange, ImageChange   1

[vagrant@openshiftdev sample-app]$ oc scale database --replicas=1
Error from server: replicationControllers "database" not found

[ironcladlou]

I'm using resource.Builder in the branch I have going, it wasn't too bad to get going.