oc: Use default resources where it makes sense

[0xmichalis]

Commands I have defaulted using the builder/RESTMapper in this PR:

• oc deploy
  (Was using both syntaxes before, now achieves it via the resource builder.)
• oadm build-chain
  (Was using both syntaxes before, now achieves it via the RESTMapper.)
• oc start-build
  (Was using only NAME syntax, now uses bc/NAME too.)
• oadm secrets add
  (Was using only TYPE/NAME syntax, now uses NAME too.)
• oc cancel-build
  (Was using only NAME syntax, now uses build/NAME too.)

@smarterclayton @fabianofranz

Fixes #4781
Fixes #1995

[deads2k]

lowercase?

[deads2k]

This command is requiring resource/name format: https://github.com/openshift/origin/blob/master/pkg/cmd/cli/secrets/add_secret_to_obj.go

[liggitt]

This command is requiring resource/name format: https://github.com/openshift/origin/blob/master/pkg/cmd/cli/secrets/add_secret_to_obj.go

For two args, no less

[0xmichalis]

[test]

[0xmichalis]

@deads2k jenkins is green, merge at your will

[liggitt]

Are there tests to make sure prior invocations still work correctly?

[0xmichalis]

Are there tests to make sure prior invocations still work correctly?

test-cmd.sh should catch such errors.

[liggitt]

I agree it should, just not sure it does :)
Can you summarize what invocations were allowed prior (with/without types, abbreviations and full resource names), and what are allowed now?
For example, abbreviations like oc secrets add sa/sa-name ... were allowed, but I don't think test-cmd.sh exercises that

[0xmichalis]

Can you summarize what invocations were allowed prior (with/without types, abbreviations and full resource names), and what are allowed now?

I've updated the first post with more details about and I've also updated test-cmd to test all cases.

[smarterclayton]

This seems like something that should be handled by resource builder?

[0xmichalis]

I am not using the resource builder here because it would try to get the buildConfig (which we are not interested in) in order to start the build. Instead, I think using the RESTMapper to resolve the resource type is enough.

[smarterclayton]

It makes me believe that what you are representing here is a common pattern
that should be abstracted either into resource.Builder (but without the
fetch) or as supporting utility code in the cmd packages.

On Oct 11, 2015, at 3:38 PM, Michail Kargakis notifications@github.com
wrote:

In pkg/cmd/cli/cmd/startbuild.go
#4947 (comment):

name := buildName
isBuild := true

• if len(name) == 0 {

• name = args[0]
  

• if len(name) == 0 && len(args[0]) > 0 {

• parts := strings.Split(args[0], "/")
  

I am not using the resource builder here because it would try to get the
buildConfig (which we are not interested in) in order to start the build.
Instead, I think using the RESTMapper to resolve the resource type is
enough.

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

[0xmichalis]

Added an utility under origin/pkg/cmd/util to handle this.

[0xmichalis]

re[test]

[0xmichalis]

go stdlib race

rere[test]

[0xmichalis]

Tagging more eyes:

@ironcladlou about deploy
@mfojtik about {start,cancel}-build

[deads2k]

I think what you really want is a way to get a canonical resource representation. Something where you get "imagestreamtag" back for "istag", "istags", "imagestreamtags" in any case. If multiple resources deal in the same kind, this breaks because "notAnIsTag/foo" would match.

I asked @smarterclayton if he knew whether it existed, he said he'd look for it/make it.

[0xmichalis]

[test]

[smarterclayton]

[test]

[smarterclayton]

Test flake in #5410

On Sat, Oct 24, 2015 at 6:00 PM, OpenShift Bot notifications@github.com
wrote:

Origin Action Required: Pull request cannot be automatically merged,
please rebase your branch from latest HEAD and push again

—
Reply to this email directly or view it on GitHub
#4947 (comment).

[liggitt]

don't you need to make sure you got back a non-empty name? I would do so after this block, to catch the case where len(args[0]) == 0

[0xmichalis]

ok

[liggitt]

one question on fromRepo, fromFile, etc interaction from @smarterclayton, then lgtm

[smarterclayton]

if len(args) > 0 || len(buildName) > 0 || len(fromFile) > 0 || len(fromDir) > 0 || len(fromRepo) > 0 {

we're safe.

On Tue, Oct 27, 2015 at 4:32 PM, Jordan Liggitt notifications@github.com
wrote:

one question on fromRepo, fromFile, etc interaction from @smarterclayton
https://github.com/smarterclayton, then lgtm

—
Reply to this email directly or view it on GitHub
#4947 (comment).

[liggitt]

move this check outside the if block to catch cases where len(name)==0, but len(args[0]) == 0, for example

[0xmichalis]

ok

[openshift-bot]

Evaluated for origin test up to e30dccd

[openshift-bot]

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

[liggitt]

LGTM, ready to merge?

[0xmichalis]

Lets do it

[liggitt]

[merge]

[openshift-bot]

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

[openshift-bot]

Evaluated for origin merge up to e30dccd