Skip to content

Commit 5fa0f68

Browse files
gabemonterogabemontero
committed
make new-app error reports clearer
1 parent 2261a32 commit 5fa0f68

File tree

11 files changed

+131
-71
lines changed

11 files changed

+131
-71
lines changed

‎pkg/cmd/cli/cmd/newapp.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ func (o *NewAppOptions) RunNewApp() error {
258258
if config.Querying() {
259259
result, err := config.RunQuery()
260260
if err != nil {
261-
return handleRunError(err, o.BaseName, o.CommandName, o.CommandPath)
261+
return handleError(err, o.BaseName, o.CommandName, o.CommandPath, config, transformRunError)
262262
}
263263

264264
if o.Action.ShouldPrint() {
@@ -271,7 +271,7 @@ func (o *NewAppOptions) RunNewApp() error {
271271
checkGitInstalled(out)
272272

273273
result, err := config.Run()
274-
if err := handleRunError(err, o.BaseName, o.CommandName, o.CommandPath); err != nil {
274+
if err := handleError(err, o.BaseName, o.CommandName, o.CommandPath, config, transformRunError); err != nil {
275275
return err
276276
}
277277

@@ -610,7 +610,7 @@ func retryBuildConfig(info *resource.Info, err error) runtime.Object {
610610
return nil
611611
}
612612

613-
func handleRunError(err error, baseName, commandName, commandPath string) error {
613+
func handleError(err error, baseName, commandName, commandPath string, config *newcmd.AppConfig, transformError func(err error, baseName, commandName, commandPath string, groups errorGroups)) error {
614614
if err == nil {
615615
return nil
616616
}
@@ -623,6 +623,14 @@ func handleRunError(err error, baseName, commandName, commandPath string) error
623623
transformError(err, baseName, commandName, commandPath, groups)
624624
}
625625
buf := &bytes.Buffer{}
626+
if len(config.ArgumentClassificationErrors) > 0 {
627+
fmt.Fprintf(buf, "Errors occurred while determining argument types:\n")
628+
for _, classErr := range config.ArgumentClassificationErrors {
629+
fmt.Fprintf(buf, fmt.Sprintf("\n%s: %v\n", classErr.Key, classErr.Value))
630+
}
631+
fmt.Fprint(buf, "\n")
632+
}
633+
fmt.Fprintln(buf, "Errors occurred during resource creation:")
626634
for _, group := range groups {
627635
fmt.Fprint(buf, kcmdutil.MultipleErrors("error: ", group.errs))
628636
if len(group.suggestion) > 0 {
@@ -647,7 +655,7 @@ func (g errorGroups) Add(group string, suggestion string, err error, errs ...err
647655
g[group] = all
648656
}
649657

650-
func transformError(err error, baseName, commandName, commandPath string, groups errorGroups) {
658+
func transformRunError(err error, baseName, commandName, commandPath string, groups errorGroups) {
651659
switch t := err.(type) {
652660
case newcmd.ErrRequiresExplicitAccess:
653661
if t.Input.Token != nil && t.Input.Token.ServiceAccount {

‎pkg/cmd/cli/cmd/newbuild.go

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package cmd
22

33
import (
4-
"bytes"
54
"fmt"
65
"io"
76
"io/ioutil"
@@ -11,7 +10,6 @@ import (
1110
"github.com/spf13/cobra"
1211

1312
kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
14-
"k8s.io/kubernetes/pkg/util/errors"
1513

1614
buildapi "github.com/openshift/origin/pkg/build/api"
1715
"github.com/openshift/origin/pkg/cmd/templates"
@@ -171,7 +169,7 @@ func (o *NewBuildOptions) RunNewBuild() error {
171169

172170
result, err := config.Run()
173171
if err != nil {
174-
return handleBuildError(err, o.BaseName, o.CommandName, o.CommandPath)
172+
return handleError(err, o.BaseName, o.CommandName, o.CommandPath, config, transformBuildError)
175173
}
176174

177175
if len(config.Labels) == 0 && len(result.Name) > 0 {
@@ -211,29 +209,6 @@ func (o *NewBuildOptions) RunNewBuild() error {
211209
return nil
212210
}
213211

214-
func handleBuildError(err error, baseName, commandName, commandPath string) error {
215-
if err == nil {
216-
return nil
217-
}
218-
errs := []error{err}
219-
if agg, ok := err.(errors.Aggregate); ok {
220-
errs = agg.Errors()
221-
}
222-
groups := errorGroups{}
223-
for _, err := range errs {
224-
transformBuildError(err, baseName, commandName, commandPath, groups)
225-
}
226-
buf := &bytes.Buffer{}
227-
for _, group := range groups {
228-
fmt.Fprint(buf, kcmdutil.MultipleErrors("error: ", group.errs))
229-
if len(group.suggestion) > 0 {
230-
fmt.Fprintln(buf)
231-
}
232-
fmt.Fprint(buf, group.suggestion)
233-
}
234-
return fmt.Errorf(buf.String())
235-
}
236-
237212
func transformBuildError(err error, baseName, commandName, commandPath string, groups errorGroups) {
238213
switch t := err.(type) {
239214
case newapp.ErrNoMatch:
@@ -261,5 +236,5 @@ func transformBuildError(err error, baseName, commandName, commandPath string, g
261236
groups.Add("", "", usageError(commandPath, newBuildNoInput, baseName, commandName))
262237
return
263238
}
264-
transformError(err, baseName, commandName, commandPath, groups)
239+
transformRunError(err, baseName, commandName, commandPath, groups)
265240
}

‎pkg/generate/app/cmd/newapp.go

Lines changed: 85 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,13 @@ type AppConfig struct {
118118

119119
OSClient client.Interface
120120
OriginNamespace string
121+
122+
ArgumentClassificationErrors []ArgumentClassificationError
123+
}
124+
125+
type ArgumentClassificationError struct {
126+
Key string
127+
Value error
121128
}
122129

123130
type ErrRequiresExplicitAccess struct {
@@ -220,30 +227,92 @@ func (c *AppConfig) SetOpenShiftClient(osclient client.Interface, OriginNamespac
220227
}
221228
}
222229

230+
func (c *AppConfig) tryToAddEnvironmentArguments(s string) bool {
231+
rc := cmdutil.IsEnvironmentArgument(s)
232+
if rc {
233+
glog.V(2).Infof("treating %s as possible environment argument\n", s)
234+
c.Environment = append(c.Environment, s)
235+
}
236+
return rc
237+
}
238+
239+
func (c *AppConfig) tryToAddSourceArguments(s string) bool {
240+
remote, rerr := app.IsRemoteRepository(s)
241+
local, derr := app.IsDirectory(s)
242+
243+
if remote || local {
244+
glog.V(2).Infof("treating %s as possible source repo\n", s)
245+
c.SourceRepositories = append(c.SourceRepositories, s)
246+
return true
247+
}
248+
249+
if rerr != nil {
250+
c.ArgumentClassificationErrors = append(c.ArgumentClassificationErrors, ArgumentClassificationError{
251+
Key: fmt.Sprintf("%s as a Git repository URL", s),
252+
Value: rerr,
253+
})
254+
}
255+
256+
if derr != nil {
257+
c.ArgumentClassificationErrors = append(c.ArgumentClassificationErrors, ArgumentClassificationError{
258+
Key: fmt.Sprintf("%s as a local directory pointing to a Git repository", s),
259+
Value: derr,
260+
})
261+
}
262+
263+
return false
264+
}
265+
266+
func (c *AppConfig) tryToAddComponentArguments(s string) bool {
267+
err := app.IsComponentReference(s)
268+
if err == nil {
269+
glog.V(2).Infof("treating %s as a component ref\n", s)
270+
c.Components = append(c.Components, s)
271+
return true
272+
}
273+
c.ArgumentClassificationErrors = append(c.ArgumentClassificationErrors, ArgumentClassificationError{
274+
Key: fmt.Sprintf("%s as a template loaded in an accessible project, an imagestream tag, or a docker image reference", s),
275+
Value: err,
276+
})
277+
278+
return false
279+
}
280+
281+
func (c *AppConfig) tryToAddTemplateArguments(s string) bool {
282+
rc, err := app.IsPossibleTemplateFile(s)
283+
if rc {
284+
glog.V(2).Infof("treating %s as possible template file\n", s)
285+
c.Components = append(c.Components, s)
286+
return true
287+
}
288+
if err != nil {
289+
c.ArgumentClassificationErrors = append(c.ArgumentClassificationErrors, ArgumentClassificationError{
290+
Key: fmt.Sprintf("%s as a template stored in a local file", s),
291+
Value: err,
292+
})
293+
}
294+
return false
295+
}
296+
223297
// AddArguments converts command line arguments into the appropriate bucket based on what they look like
224298
func (c *AppConfig) AddArguments(args []string) []string {
225299
unknown := []string{}
300+
c.ArgumentClassificationErrors = []ArgumentClassificationError{}
226301
for _, s := range args {
302+
if len(s) == 0 {
303+
continue
304+
}
305+
227306
switch {
228-
case cmdutil.IsEnvironmentArgument(s):
229-
glog.V(2).Infof("treating %s as possible environment argument\n", s)
230-
c.Environment = append(c.Environment, s)
231-
case app.IsPossibleSourceRepository(s):
232-
glog.V(2).Infof("treating %s as possible source repo\n", s)
233-
c.SourceRepositories = append(c.SourceRepositories, s)
234-
case app.IsComponentReference(s):
235-
glog.V(2).Infof("treating %s as a component ref\n", s)
236-
c.Components = append(c.Components, s)
237-
case app.IsPossibleTemplateFile(s):
238-
glog.V(2).Infof("treating %s as possible template file\n", s)
239-
c.Components = append(c.Components, s)
307+
case c.tryToAddEnvironmentArguments(s):
308+
case c.tryToAddSourceArguments(s):
309+
case c.tryToAddComponentArguments(s):
310+
case c.tryToAddTemplateArguments(s):
240311
default:
241312
glog.V(2).Infof("treating %s as unknown\n", s)
242-
if len(s) == 0 {
243-
break
244-
}
245313
unknown = append(unknown, s)
246314
}
315+
247316
}
248317
return unknown
249318
}
@@ -644,7 +713,7 @@ func (c *AppConfig) Run() (*AppResult, error) {
644713
// TODO: I don't belong here
645714
c.ensureDockerSearch()
646715

647-
resolved, err := Resolve(&c.Resolvers, &c.ComponentInputs, &c.GenerationInputs)
716+
resolved, err := Resolve(c)
648717
if err != nil {
649718
return nil, err
650719
}

‎pkg/generate/app/cmd/newapp_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ func TestBuildTemplates(t *testing.T) {
195195
continue
196196
}
197197

198-
resolved, err := Resolve(&appCfg.Resolvers, &appCfg.ComponentInputs, &appCfg.GenerationInputs)
198+
resolved, err := Resolve(&appCfg)
199199
if err != nil {
200200
t.Errorf("%s: Unexpected error: %v", n, err)
201201
continue

‎pkg/generate/app/cmd/resolve.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,10 @@ type ResolvedComponents struct {
9595

9696
// Resolve transforms unstructured inputs (component names, templates, images) into
9797
// a set of resolved components, or returns an error.
98-
func Resolve(r *Resolvers, c *ComponentInputs, g *GenerationInputs) (*ResolvedComponents, error) {
98+
func Resolve(appConfig *AppConfig) (*ResolvedComponents, error) {
99+
r := &appConfig.Resolvers
100+
c := &appConfig.ComponentInputs
101+
g := &appConfig.GenerationInputs
99102
b := &app.ReferenceBuilder{}
100103

101104
if err := AddComponentInputsToRefBuilder(b, r, c, g); err != nil {
@@ -116,7 +119,7 @@ func Resolve(r *Resolvers, c *ComponentInputs, g *GenerationInputs) (*ResolvedCo
116119
}
117120

118121
if g.Strategy != generate.StrategyUnspecified && len(repositories) == 0 && !g.BinaryBuild {
119-
return nil, errors.New("when --strategy is specified you must provide at least one source code location")
122+
return nil, errors.New("--strategy is specified and none of the arguments provided could be classified as a source code location")
120123
}
121124

122125
if g.BinaryBuild && (len(repositories) > 0 || components.HasSource()) {

‎pkg/generate/app/componentref.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,16 @@ import (
1010
"k8s.io/kubernetes/pkg/util/errors"
1111
)
1212

13-
// IsComponentReference returns true if the provided string appears to be a reference to a source repository
13+
// IsComponentReference returns an error if the provided string does not appear to be a reference to a source repository
1414
// on disk, at a URL, a docker image name (which might be on a Docker registry or an OpenShift image stream),
1515
// or a template.
16-
func IsComponentReference(s string) bool {
16+
func IsComponentReference(s string) error {
1717
if len(s) == 0 {
18-
return false
18+
return fmt.Errorf("empty string provided to component reference check")
1919
}
2020
all := strings.Split(s, "+")
2121
_, _, _, err := componentWithSource(all[0])
22-
return err == nil
22+
return err
2323
}
2424

2525
// componentWithSource parses the provided string and returns an image component

‎pkg/generate/app/file.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@ import (
55
)
66

77
// isFile returns true if the passed-in argument is a file in the filesystem
8-
func isFile(name string) bool {
8+
func isFile(name string) (bool, error) {
99
info, err := os.Stat(name)
10-
return err == nil && !info.IsDir()
10+
return err == nil && !info.IsDir(), err
1111
}
1212

13-
// isDirectory returns true if the passed-in argument is a directory in the filesystem
14-
func isDirectory(name string) bool {
13+
// IsDirectory returns true if the passed-in argument is a directory in the filesystem
14+
func IsDirectory(name string) (bool, error) {
1515
info, err := os.Stat(name)
16-
return err == nil && info.IsDir()
16+
return err == nil && info.IsDir(), err
1717
}

‎pkg/generate/app/sourcelookup.go

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -66,30 +66,25 @@ func (d dockerfileContents) Contents() string {
6666
return d.contents
6767
}
6868

69-
// IsPossibleSourceRepository checks whether the provided string is a source repository or not
70-
func IsPossibleSourceRepository(s string) bool {
71-
return IsRemoteRepository(s) || isDirectory(s)
72-
}
73-
7469
// IsRemoteRepository checks whether the provided string is a remote repository or not
75-
func IsRemoteRepository(s string) bool {
70+
func IsRemoteRepository(s string) (bool, error) {
7671
if !s2igit.New(s2iutil.NewFileSystem()).ValidCloneSpecRemoteOnly(s) {
7772
glog.V(5).Infof("%s is not a valid remote git clone spec", s)
78-
return false
73+
return false, nil
7974
}
8075
url, err := url.Parse(s)
8176
if err != nil {
8277
glog.V(5).Infof("%s is not a valid url: %v", s, err)
83-
return false
78+
return false, err
8479
}
8580
url.Fragment = ""
8681
gitRepo := git.NewRepository()
8782
if _, _, err := gitRepo.ListRemote(url.String()); err != nil {
8883
glog.V(5).Infof("could not list git remotes for %s: %v", s, err)
89-
return false
84+
return false, err
9085
}
9186
glog.V(5).Infof("%s is a valid remote git repository", s)
92-
return true
87+
return true, nil
9388
}
9489

9590
// SourceRepository represents a code repository that may be the target of a build.

‎pkg/generate/app/templatelookup.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ func (r TemplateSearcher) Search(precise bool, terms ...string) (ComponentMatche
9595
}
9696

9797
// IsPossibleTemplateFile returns true if the argument can be a template file
98-
func IsPossibleTemplateFile(value string) bool {
98+
func IsPossibleTemplateFile(value string) (bool, error) {
9999
return isFile(value)
100100
}
101101

‎test/cmd/newapp.sh

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,16 @@ os::cmd::expect_success_and_text 'oc new-app mysql -o yaml --as-test' 'test: tru
4040
# docker strategy with repo that has no dockerfile
4141
os::cmd::expect_failure_and_text 'oc new-app https://github.com/openshift/nodejs-ex --strategy=docker' 'No Dockerfile was found'
4242

43+
# repo related error message validation
44+
os::cmd::expect_failure_and_text 'oc new-app mysql-persisten mysql' 'mysql-persisten as a local directory'
45+
os::cmd::expect_failure_and_text 'oc new-app mysql-persisten mysql' 'mysql as a local directory'
46+
os::cmd::expect_failure_and_text 'oc new-app --strategy=docker https://192.30.253.113/openshift/ruby-hello-world.git' 'as a Git repository URL: '
47+
os::cmd::expect_failure_and_text 'oc new-app https://www.google.com/openshift/nodejs-e' 'as a Git repository URL: '
48+
os::cmd::expect_failure_and_text 'oc new-app https://githb.com/openshift/nodejs-e' 'as a Git repository URL: '
49+
os::cmd::expect_failure_and_text 'oc new-build --strategy=docker https://192.30.253.113/openshift/ruby-hello-world.git' 'as a Git repository URL: '
50+
os::cmd::expect_failure_and_text 'oc new-build https://www.google.com/openshift/nodejs-e' 'as a Git repository URL: '
51+
os::cmd::expect_failure_and_text 'oc new-build https://githb.com/openshift/nodejs-e' 'as a Git repository URL: '
52+
4353
# check label creation
4454
os::cmd::try_until_success 'oc get imagestreamtags php:latest'
4555
os::cmd::try_until_success 'oc get imagestreamtags php:5.5'

‎test/extended/builds/new_app.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,6 @@ var _ = g.Describe("[builds][Conformance] oc new-app", func() {
4747
g.By("calling oc new-app")
4848
out, err := oc.Run("new-app").Args("https://github.com/openshift/nodejs-ex", "--name", a59).Output()
4949
o.Expect(err).To(o.HaveOccurred())
50-
o.Expect(out).To(o.HavePrefix("error: invalid name: "))
50+
o.Expect(out).To(o.ContainSubstring("error: invalid name: "))
5151
})
5252
})

0 commit comments

Comments
 (0)