Skip to content

oc: Use default resources where it makes sense #4947

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

Merged
openshift-bot merged 1 commit into from
Oct 31, 2015
Merged

oc: Use default resources where it makes sense #4947

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
14 changes: 12 additions & 2 deletions pkg/cmd/cli/cmd/cancelbuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
kapi "k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/errors"
cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
"k8s.io/kubernetes/pkg/kubectl/resource"

buildapi "github.com/openshift/origin/pkg/build/api"
"github.com/openshift/origin/pkg/cmd/util/clientcmd"
Expand Down Expand Up @@ -68,11 +69,20 @@ func RunCancelBuild(f *clientcmd.Factory, out io.Writer, cmd *cobra.Command, arg
return err
}
buildClient := client.Builds(namespace)
build, err := buildClient.Get(buildName)

mapper, typer := f.Object()
obj, err := resource.NewBuilder(mapper, typer, f.ClientMapperForCommand()).
NamespaceParam(namespace).
ResourceNames("builds", buildName).
SingleResourceType().
Do().Object()
if err != nil {
return err
}

build, ok := obj.(*buildapi.Build)
if !ok {
return fmt.Errorf("%q is not a valid build", buildName)
}
if !isBuildCancellable(build) {
return nil
}
Expand Down
14 changes: 5 additions & 9 deletions pkg/cmd/cli/cmd/deploy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func NewCmdDeploy(fullName string, f *clientcmd.Factory, out io.Writer) *cobra.C

func (o *DeployOptions) Complete(f *clientcmd.Factory, args []string, out io.Writer) error {
if len(args) > 1 {
return errors.New("only one deploymentConfig name is supported as argument.")
return errors.New("only one deployment config name is supported as argument.")
}
var err error

Expand All @@ -139,19 +139,15 @@ func (o *DeployOptions) Complete(f *clientcmd.Factory, args []string, out io.Wri
o.out = out

if len(args) > 0 {
Copy link
Contributor

@liggitt liggitt Oct 19, 2015

Choose a reason for hiding this comment

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

are we discarding args if they give us more than 1? should we error on that?

Copy link
Contributor Author

@0xmichalis 0xmichalis Oct 19, 2015

Choose a reason for hiding this comment

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

We already do; see

origin/pkg/cmd/cli/cmd/deploy.go

Lines 121 to 123 in c4c0150

if len(args) > 1 {
return errors.New("only one deploymentConfig name is supported as argument.")
}

Copy link
Contributor

@liggitt liggitt Oct 19, 2015

Choose a reason for hiding this comment

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

oh, ha... that's 18 whole lines away... obviously I can't see that far :)

name := args[0]
if strings.Index(name, "/") == -1 {
name = fmt.Sprintf("dc/%s", name)
}
o.deploymentConfigName = name
o.deploymentConfigName = args[0]
}

return nil
}

func (o DeployOptions) Validate() error {
if len(o.deploymentConfigName) == 0 {
return errors.New("a deploymentConfig name is required.")
return errors.New("a deployment config name is required.")
}
numOptions := 0
if o.deployLatest {
Expand All @@ -175,7 +171,7 @@ func (o DeployOptions) Validate() error {
func (o DeployOptions) RunDeploy() error {
r := o.builder.
NamespaceParam(o.namespace).
ResourceTypeOrNameArgs(false, o.deploymentConfigName).
ResourceNames("deploymentconfigs", o.deploymentConfigName).
SingleResourceType().
Do()
resultObj, err := r.Object()
Expand All @@ -184,7 +180,7 @@ func (o DeployOptions) RunDeploy() error {
}
config, ok := resultObj.(*deployapi.DeploymentConfig)
if !ok {
return fmt.Errorf("%s is not a valid deploymentconfig", o.deploymentConfigName)
return fmt.Errorf("%s is not a valid deployment config", o.deploymentConfigName)
}

switch {
Expand Down
58 changes: 41 additions & 17 deletions pkg/cmd/cli/cmd/startbuild.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (

buildapi "github.com/openshift/origin/pkg/build/api"
osclient "github.com/openshift/origin/pkg/client"
osutil "github.com/openshift/origin/pkg/cmd/util"
"github.com/openshift/origin/pkg/cmd/util/clientcmd"
"github.com/openshift/origin/pkg/generate/git"
"github.com/openshift/source-to-image/pkg/tar"
Expand Down Expand Up @@ -129,27 +130,44 @@ func RunStartBuild(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra
return cmdutil.UsageError(cmd, "Must pass a name of a build config or specify build name with '--from-build' flag")
}

name := buildName
isBuild := true
namespace, _, err := f.DefaultNamespace()
if err != nil {
return err
}

var (
name = buildName
resource = "builds"
)

if len(name) == 0 && len(args) > 0 && len(args[0]) > 0 {
mapper, _ := f.Object()
resource, name, err = osutil.ResolveResource("buildconfigs", args[0], mapper)
Copy link
Contributor

@liggitt liggitt Oct 27, 2015

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@0xmichalis 0xmichalis Oct 27, 2015

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
}
switch resource {
case "buildconfigs":
Copy link
Contributor

@liggitt liggitt Oct 19, 2015

Choose a reason for hiding this comment

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

add a // no special handling required comment to make it clear this isn't intended to be a fall-through case

Copy link
Contributor Author

@0xmichalis 0xmichalis Oct 19, 2015

Choose a reason for hiding this comment

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

ok

// no special handling required
case "builds":
return fmt.Errorf("use --from-build to rerun your builds")
default:
return fmt.Errorf("invalid resource provided: %s", resource)
}
}
if len(name) == 0 {
name = args[0]
isBuild = false
return fmt.Errorf("a resource name is required either as an argument or by using --from-build")
}

if webhooks.Provided() {
return RunListBuildWebHooks(f, out, cmd.Out(), name, isBuild, webhooks.String())
return RunListBuildWebHooks(f, out, cmd.Out(), name, resource, webhooks.String())
}

client, _, err := f.Clients()
if err != nil {
return err
}

namespace, _, err := f.DefaultNamespace()
if err != nil {
return err
}

request := &buildapi.BuildRequest{
ObjectMeta: kapi.ObjectMeta{Name: name},
}
Expand All @@ -166,7 +184,7 @@ func RunStartBuild(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra

var newBuild *buildapi.Build
switch {
case !isBuild && (len(fromFile) > 0 || len(fromDir) > 0 || len(fromRepo) > 0):
case len(args) > 0 && (len(fromFile) > 0 || len(fromDir) > 0 || len(fromRepo) > 0):
request := &buildapi.BinaryBuildRequestOptions{
ObjectMeta: kapi.ObjectMeta{
Name: name,
Expand All @@ -177,16 +195,16 @@ func RunStartBuild(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra
if newBuild, err = streamPathToBuild(git, in, cmd.Out(), client.BuildConfigs(namespace), fromDir, fromFile, fromRepo, request); err != nil {
return err
}

case isBuild:
case resource == "builds":
if newBuild, err = client.Builds(namespace).Clone(request); err != nil {
return err
}

default:
case resource == "buildconfigs":
if newBuild, err = client.BuildConfigs(namespace).Instantiate(request); err != nil {
return err
}
Copy link
Contributor

@liggitt liggitt Oct 19, 2015

Choose a reason for hiding this comment

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

add a default case that errors

Copy link
Contributor Author

@0xmichalis 0xmichalis Oct 19, 2015

Choose a reason for hiding this comment

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

We already have that covered in L126, but uf you really want to check it here too, I can add it.

default:
return fmt.Errorf("invalid resource provided: %s", resource)
}

fmt.Fprintln(out, newBuild.Name)
Expand Down Expand Up @@ -240,7 +258,7 @@ func RunStartBuild(f *clientcmd.Factory, in io.Reader, out io.Writer, cmd *cobra
}

// RunListBuildWebHooks prints the webhooks for the provided build config.
func RunListBuildWebHooks(f *clientcmd.Factory, out, errOut io.Writer, name string, isBuild bool, webhookFilter string) error {
func RunListBuildWebHooks(f *clientcmd.Factory, out, errOut io.Writer, name, resource, webhookFilter string) error {
generic, github := false, false
prefix := false
switch webhookFilter {
Expand All @@ -263,7 +281,10 @@ func RunListBuildWebHooks(f *clientcmd.Factory, out, errOut io.Writer, name stri
return err
}

if isBuild {
switch resource {
case "buildconfigs":
// no special handling required
case "builds":
build, err := client.Builds(namespace).Get(name)
if err != nil {
return err
Expand All @@ -276,7 +297,10 @@ func RunListBuildWebHooks(f *clientcmd.Factory, out, errOut io.Writer, name stri
namespace = ref.Namespace
}
name = ref.Name
default:
return fmt.Errorf("invalid resource provided: %s", resource)
}

config, err := client.BuildConfigs(namespace).Get(name)
if err != nil {
return err
Expand Down
4 changes: 2 additions & 2 deletions pkg/cmd/cli/secrets/add_secret_to_obj.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ func (o AddSecretOptions) Validate() error {
func (o AddSecretOptions) AddSecrets() error {
r := resource.NewBuilder(o.Mapper, o.Typer, o.ClientMapper).
NamespaceParam(o.Namespace).
ResourceTypeOrNameArgs(false, o.TargetName).
ResourceNames("serviceaccounts", o.TargetName).
SingleResourceType().
Do()
if r.Err() != nil {
Expand Down Expand Up @@ -223,7 +223,7 @@ func (o AddSecretOptions) addSecretsToServiceAccount(serviceaccount *kapi.Servic
func (o AddSecretOptions) getSecrets() ([]*kapi.Secret, error) {
r := resource.NewBuilder(o.Mapper, o.Typer, o.ClientMapper).
NamespaceParam(o.Namespace).
ResourceTypeOrNameArgs(false, o.SecretNames...).
ResourceNames("secrets", o.SecretNames...).
SingleResourceType().
Do()
if r.Err() != nil {
Expand Down
60 changes: 22 additions & 38 deletions pkg/cmd/experimental/buildchain/buildchain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

"github.com/openshift/origin/pkg/client"
"github.com/openshift/origin/pkg/cmd/cli/describe"
osutil "github.com/openshift/origin/pkg/cmd/util"
"github.com/openshift/origin/pkg/cmd/util/clientcmd"
imageapi "github.com/openshift/origin/pkg/image/api"
imagegraph "github.com/openshift/origin/pkg/image/graph/nodes"
Expand Down Expand Up @@ -43,7 +44,6 @@ const BuildChainRecommendedCommandName = "build-chain"
// BuildChainOptions contains all the options needed for build-chain
type BuildChainOptions struct {
name string
tag string

defaultNamespace string
namespaces sets.String
Expand All @@ -62,7 +62,7 @@ func NewCmdBuildChain(name, fullName string, f *clientcmd.Factory, out io.Writer
namespaces: sets.NewString(),
}
cmd := &cobra.Command{
Use: "build-chain [IMAGESTREAM:TAG]",
Use: "build-chain IMAGESTREAMTAG",
Short: "Output the inputs and dependencies of your builds",
Long: buildChainLong,
Example: fmt.Sprintf(buildChainExample, fullName),
Expand All @@ -84,7 +84,7 @@ func NewCmdBuildChain(name, fullName string, f *clientcmd.Factory, out io.Writer
// Complete completes the required options for build-chain
func (o *BuildChainOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, args []string, out io.Writer) error {
if len(args) != 1 {
return cmdutil.UsageError(cmd, "Must pass an image stream name and optionally a tag. In case of an empty tag, 'latest' will be used.")
return cmdutil.UsageError(cmd, "Must pass an image stream tag. If only an image stream name is specified, 'latest' will be used for the tag.")
}

// Setup client
Expand All @@ -94,12 +94,20 @@ func (o *BuildChainOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, a
}
o.c, o.t = oc, oc

// Parse user input
o.name, o.tag, err = buildChainInput(args[0])
resource := ""
mapper, _ := f.Object()
resource, o.name, err = osutil.ResolveResource("imagestreamtags", args[0], mapper)
if err != nil {
return cmdutil.UsageError(cmd, err.Error())
return err
}

switch resource {
case "imagestreamtags":
o.name = imageapi.NormalizeImageStreamTag(o.name)
glog.V(4).Infof("Using %q as the image stream tag to look dependencies for", o.name)
default:
return fmt.Errorf("invalid resource provided: %s", resource)
}
glog.V(4).Infof("Using '%s:%s' as the image stream tag to look dependencies for", o.name, o.tag)

// Setup namespace
if o.allNamespaces {
Expand All @@ -120,7 +128,7 @@ func (o *BuildChainOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, a
}

o.defaultNamespace = namespace
glog.V(4).Infof("Using %q as the namespace for '%s:%s'", o.defaultNamespace, o.name, o.tag)
glog.V(4).Infof("Using %q as the namespace for %q", o.defaultNamespace, o.name)
o.namespaces.Insert(namespace)
glog.V(4).Infof("Will look for deps in %s", strings.Join(o.namespaces.List(), ","))

Expand All @@ -130,10 +138,7 @@ func (o *BuildChainOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, a
// Validate returns validation errors regarding build-chain
func (o *BuildChainOptions) Validate() error {
if len(o.name) == 0 {
return fmt.Errorf("image stream name cannot be empty")
}
if len(o.tag) == 0 {
o.tag = imageapi.DefaultImageTag
return fmt.Errorf("image stream tag cannot be empty")
}
if len(o.defaultNamespace) == 0 {
return fmt.Errorf("default namespace cannot be empty")
Expand All @@ -153,15 +158,17 @@ func (o *BuildChainOptions) Validate() error {
// RunBuildChain contains all the necessary functionality for the OpenShift
// experimental build-chain command
func (o *BuildChainOptions) RunBuildChain() error {
ist := imagegraph.MakeImageStreamTagObjectMeta(o.defaultNamespace, o.name, o.tag)
ist := imagegraph.MakeImageStreamTagObjectMeta2(o.defaultNamespace, o.name)

desc, err := describe.NewChainDescriber(o.c, o.namespaces, o.output).Describe(ist, !o.triggerOnly)
if err != nil {
if _, isNotFoundErr := err.(describe.NotFoundErr); isNotFoundErr {
name, tag, _ := imageapi.SplitImageStreamTag(o.name)
// Try to get the imageStreamTag via a direct GET
if _, getErr := o.t.ImageStreamTags(o.defaultNamespace).Get(o.name, o.tag); getErr != nil {
if _, getErr := o.t.ImageStreamTags(o.defaultNamespace).Get(name, tag); getErr != nil {
return getErr
}
fmt.Printf("Image stream tag '%s:%s' in %q doesn't have any dependencies.\n", o.name, o.tag, o.defaultNamespace)
fmt.Printf("Image stream tag %q in %q doesn't have any dependencies.\n", o.name, o.defaultNamespace)
return nil
}
return err
Expand All @@ -171,26 +178,3 @@ func (o *BuildChainOptions) RunBuildChain() error {

return nil
}

// buildChainInput parses user input and returns a stream name, a tag
// and an error if any
func buildChainInput(input string) (string, string, error) {
// Split name and tag
name, tag, _ := imageapi.SplitImageStreamTag(input)

// Support resource type/name syntax
// TODO: Use the RESTMapper to resolve this
resource := strings.Split(name, "/")
switch len(resource) {
case 1:
case 2:
resourceType := resource[0]
if resourceType != "istag" && resourceType != "imagestreamtag" {
return "", "", fmt.Errorf("invalid resource type %q", resourceType)
}
default:
return "", "", fmt.Errorf("invalid image stream name %q", name)
}

return name, tag, nil
}
29 changes: 29 additions & 0 deletions pkg/cmd/util/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package util

import (
"errors"
"fmt"
"io"
"path/filepath"
"strings"

"github.com/spf13/cobra"

"k8s.io/kubernetes/pkg/api/meta"
kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
)

Expand All @@ -31,3 +33,30 @@ func GetDisplayFilename(filename string) string {

return filename
}

// ResolveResource returns the resource type and name of the resourceString.
// If the resource string has no specified type, defaultResource will be returned.
func ResolveResource(defaultResource, resourceString string, mapper meta.RESTMapper) (string, string, error) {
Copy link
Contributor

@liggitt liggitt Oct 19, 2015

Choose a reason for hiding this comment

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

need lots of tests around this. Off the top of my head:

// invalid cases
"", "a/b/c"   -> "", "", error about unknown format
"", "foo/bar" -> "", "", error about unknown resource foo

// empty resource string cases
"",    ""    -> "",    "",    nil
"",    "bar" -> "",    "bar", nil
"foo", "bar" -> "foo", "bar", nil

// shortname, case-insensitive (preserving name case), singular/plural K8s resources from resource string
"foo","rc/bar"                     -> "replicationcontrollers", "bar", nil
"foo","replicationcontroller/bar"  -> "replicationcontrollers", "bar", nil
"foo","replicationcontrollers/bar" -> "replicationcontrollers", "bar", nil
"foo","ReplicationControllers/bar" -> "replicationcontrollers", "bar", nil
"foo","ReplicationControllers/Bar" -> "replicationcontrollers", "Bar", nil

// shortname, case-insensitive (preserving name case), singular/plural origin resources from resource string
"foo","is/bar"           -> "imagestreams", "bar", nil
"foo","imagestream/bar"  -> "imagestreams", "bar", nil
"foo","imagestream/bar"  -> "imagestreams", "bar", nil
"foo","imagestreams/bar" -> "imagestreams", "bar", nil
"foo","ImageStreams/bar" -> "imagestreams", "bar", nil
"foo","ImageStreams/Bar" -> "imagestreams", "Bar", nil

Copy link
Contributor Author

@0xmichalis 0xmichalis Oct 19, 2015

Choose a reason for hiding this comment

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

done

if mapper == nil {
return "", "", errors.New("mapper cannot be nil")
}

var name string
parts := strings.Split(resourceString, "/")
switch len(parts) {
case 1:
name = parts[0]
Copy link
Contributor

@deads2k deads2k Oct 12, 2015

Choose a reason for hiding this comment

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

Shouldn't the default kind be applied for cases where you only have a name?

Copy link
Contributor Author

@0xmichalis 0xmichalis Oct 12, 2015

Choose a reason for hiding this comment

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

What you are asking about is ResourceNames in the resource builder. This is different.

case 2:
_, kind, err := mapper.VersionAndKindForResource(parts[0])
Copy link
Contributor

@liggitt liggitt Oct 15, 2015

Choose a reason for hiding this comment

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

I think this validates the resource is known, and handles expanding short forms to long forms, which is good

if err != nil {
return "", "", err
}
name = parts[1]
resource, _ := meta.KindToResource(kind, false)
return resource, name, nil
default:
return "", "", fmt.Errorf("invalid resource format: %s", resourceString)
}

return defaultResource, name, nil
}
Loading