Simplify rollback arguments

[ironcladlou]

Make the rollback command speak in terms of deployment configs and
versions, instead of deployments which aren't formal API resources.

This keeps the command consistent with other deployment related APIs
and commands.

Rollback now accepts a deployment config and version for the rollback
target; if no version is supplied, the last completed deployment will
be inferred.

Closes #3338

[ironcladlou]

@deads2k @Kargakis PTAL.

[deads2k]

I like this better, but it breaks existing scripts. @smarterclayton what's our stance on that?

[smarterclayton]

Don't

On Jul 9, 2015, at 10:58 AM, David Eads [email protected] wrote:

In pkg/cmd/cli/cmd/rollback.go
#3643 (comment):

@@ -57,14 +60,14 @@ func NewCmdRollback(fullName string, f *clientcmd.Factory, out io.Writer) *cobra
}

cmd := &cobra.Command{

• Use:     "rollback DEPLOYMENT",
  

• Use:     "rollback [dc/]config-name",
  

I like this better, but it breaks existing scripts. @smarterclayton
https://github.com/smarterclayton what's our stance on that?

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/3643/files#r34264171.

[deads2k]

Don't

@ironcladlou I'm not sure how you can distinguish the different cases (old arg versus new arg). Also, the fact that hack/test-cmd.sh didn't require any changes suggests this needs to be added there.

[ironcladlou]

We can never change the syntax of this command? What is the proposed syntax that gets us to a place where we can accept a config? rollback --to-config=config --to-version=N (no arg[0]?)

[smarterclayton]

Add new flags, deprecate old ones, or allow a wider range of options. But
we must manage cli syntax change exactly like API or config change -
slowly, with explicit deprecation and management.

On Jul 9, 2015, at 12:52 PM, Dan Mace [email protected] wrote:

In pkg/cmd/cli/cmd/rollback.go
#3643 (comment):

@@ -57,14 +60,14 @@ func NewCmdRollback(fullName string, f *clientcmd.Factory, out io.Writer) *cobra
}

cmd := &cobra.Command{

• Use:     "rollback DEPLOYMENT",
  

• Use:     "rollback [dc/]config-name",
  

We can never change the syntax of this command? What is the proposed syntax
that gets us to a place where we can accept a config? rollback
--to-config=config --to-version=N (no arg[0]?)

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/3643/files#r34276449.

[ironcladlou]

Here's what @deads2k and I are proposing now:

rollback <name>

We'll try and use <name> as an RC first, then a DC, and use whatever we find. The possibility of a DC and RC having the same name within a namespace seems extremely low, since RC names are <config-name>-<version>.

Both rc/ and dc/ prefixes would be supported.

[smarterclayton]

That's reasonable to me

[deads2k]

Since this is a reasonably significant refactor, are you willing to follow the RollbackOptions, Complete, Validate, RunRollback pattern?

[ironcladlou]

Since this is a reasonably significant refactor, are you willing to follow the RollbackOptions, Complete, Validate, RunRollback pattern?

I'm not sure what pattern you're referring to. Details?

[deads2k]

couldn't you just default to-version to 0 and then say that to-version=0 means auto-detect?

[ironcladlou]

You're right; changed this to always use the argument value (which defaults to 0) and updated the argument's description.

[0xmichalis]

Since this is a reasonably significant refactor, are you willing to follow the RollbackOptions, Complete, Validate, RunRollback pattern?

+1 to this. Or at least factor out RunRollback.

[0xmichalis]

I'm not sure what pattern you're referring to. Details?

https://github.com/openshift/origin/pull/3470/files?short_path=6191d79#diff-6191d79908dca377719733511c6b39e9 - Check the Command Structure part

[deads2k]

I'm not sure what pattern you're referring to. Details?

https://github.com/openshift/origin/blob/master/pkg/cmd/cli/secrets/new.go

[ironcladlou]

Re-re-factored... PTAL. test-cmd.sh coverage is still outstanding.

[ironcladlou]

Added basic test coverage in test-cmd.sh.

[deads2k]

Edit help text to say that this is ignored (invalid?) when specifying an rc?

[ironcladlou]

Fixed

[deads2k]

lgtm. Please open the issues before you merge so we don't lose track of them.

[ironcladlou]

Okay, can somebody tag this?

[smarterclayton]

LGTM [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/2704/) (Image: devenv-fedora_1972)

[openshift-bot]

[Test]ing while waiting on the merge queue

[openshift-bot]

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/3595/)

[openshift-bot]

Evaluated for origin up to 14c47aa