Skip to content

Commit 023acd6

Browse files
gabemonterogabemontero
committed
make new-app error reports clearer
1 parent 19aa349 commit 023acd6

File tree

10 files changed

+118
-44
lines changed

10 files changed

+118
-44
lines changed

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ func (o *NewAppOptions) RunNewApp() error {
247247
if config.Querying() {
248248
result, err := config.RunQuery()
249249
if err != nil {
250-
return handleRunError(err, o.BaseName, o.CommandName, o.CommandPath)
250+
return handleRunError(err, o.BaseName, o.CommandName, o.CommandPath, config)
251251
}
252252

253253
if o.Action.ShouldPrint() {
@@ -260,7 +260,7 @@ func (o *NewAppOptions) RunNewApp() error {
260260
checkGitInstalled(out)
261261

262262
result, err := config.Run()
263-
if err := handleRunError(err, o.BaseName, o.CommandName, o.CommandPath); err != nil {
263+
if err := handleRunError(err, o.BaseName, o.CommandName, o.CommandPath, config); err != nil {
264264
return err
265265
}
266266

@@ -599,7 +599,7 @@ func retryBuildConfig(info *resource.Info, err error) runtime.Object {
599599
return nil
600600
}
601601

602-
func handleRunError(err error, baseName, commandName, commandPath string) error {
602+
func handleRunError(err error, baseName, commandName, commandPath string, config *newcmd.AppConfig) error {
603603
if err == nil {
604604
return nil
605605
}
@@ -612,7 +612,15 @@ func handleRunError(err error, baseName, commandName, commandPath string) error
612612
transformError(err, baseName, commandName, commandPath, groups)
613613
}
614614
buf := &bytes.Buffer{}
615+
if len(config.ArgumentClassificationErrors) > 0 {
616+
fmt.Fprintf(buf, "Attempts to classify the arguments resulted in these errors:\n")
617+
for key, value := range config.ArgumentClassificationErrors {
618+
fmt.Fprintf(buf, fmt.Sprintf("\n%s: %v\n", key, value))
619+
}
620+
fmt.Fprint(buf, "\nThe following errors occurred during application creation:\n")
621+
}
615622
for _, group := range groups {
623+
//TODO how coupled are we to kcmdutil.MultipleErrors ... format is compressed, non-optimal readability
616624
fmt.Fprint(buf, kcmdutil.MultipleErrors("error: ", group.errs))
617625
if len(group.suggestion) > 0 {
618626
fmt.Fprintln(buf)

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ func (o *NewBuildOptions) RunNewBuild() error {
218218

219219
result, err := config.Run()
220220
if err != nil {
221-
return handleBuildError(err, o.BaseName, o.CommandName, o.CommandPath)
221+
return handleBuildError(err, o.BaseName, o.CommandName, o.CommandPath, config)
222222
}
223223

224224
if len(config.Labels) == 0 && len(result.Name) > 0 {
@@ -258,7 +258,7 @@ func (o *NewBuildOptions) RunNewBuild() error {
258258
return nil
259259
}
260260

261-
func handleBuildError(err error, baseName, commandName, commandPath string) error {
261+
func handleBuildError(err error, baseName, commandName, commandPath string, config *newcmd.AppConfig) error {
262262
if err == nil {
263263
return nil
264264
}
@@ -271,7 +271,15 @@ func handleBuildError(err error, baseName, commandName, commandPath string) erro
271271
transformBuildError(err, baseName, commandName, commandPath, groups)
272272
}
273273
buf := &bytes.Buffer{}
274+
if len(config.ArgumentClassificationErrors) > 0 {
275+
fmt.Fprintf(buf, "Attempts to classify the arguments resulted in these errors:\n")
276+
for key, value := range config.ArgumentClassificationErrors {
277+
fmt.Fprintf(buf, fmt.Sprintf("\n%s: %v\n", key, value))
278+
}
279+
fmt.Fprint(buf, "\nThe following errors occurred when creating a build from source code:\n")
280+
}
274281
for _, group := range groups {
282+
//TODO how coupled are we to kcmdutil.MultipleErrors ... format is compressed, non-optimal readability
275283
fmt.Fprint(buf, kcmdutil.MultipleErrors("error: ", group.errs))
276284
if len(group.suggestion) > 0 {
277285
fmt.Fprintln(buf)

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

Lines changed: 68 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,8 @@ type AppConfig struct {
117117

118118
OSClient client.Interface
119119
OriginNamespace string
120+
121+
ArgumentClassificationErrors map[string]error
120122
}
121123

122124
type ErrRequiresExplicitAccess struct {
@@ -219,30 +221,80 @@ func (c *AppConfig) SetOpenShiftClient(osclient client.Interface, OriginNamespac
219221
}
220222
}
221223

224+
func (c *AppConfig) addEnvironmentArguments(s string) bool {
225+
rc := cmdutil.IsEnvironmentArgument(s)
226+
if rc {
227+
glog.V(2).Infof("treating %s as possible environment argument\n", s)
228+
c.Environment = append(c.Environment, s)
229+
}
230+
return rc
231+
}
232+
233+
func (c *AppConfig) addSourceArguments(s string) bool {
234+
remote, rerr := app.IsRemoteRepository(s)
235+
local, derr := app.IsDirectory(s)
236+
237+
if remote || local {
238+
glog.V(2).Infof("treating %s as possible source repo\n", s)
239+
c.SourceRepositories = append(c.SourceRepositories, s)
240+
return true
241+
}
242+
243+
if rerr != nil {
244+
c.ArgumentClassificationErrors["Remote access"] = rerr
245+
}
246+
247+
if derr != nil {
248+
c.ArgumentClassificationErrors["Local access"] = derr
249+
}
250+
251+
return false
252+
}
253+
254+
func (c *AppConfig) addComponentArguments(s string) bool {
255+
err := app.IsComponentReference(s)
256+
if err == nil {
257+
glog.V(2).Infof("treating %s as a component ref\n", s)
258+
c.Components = append(c.Components, s)
259+
return true
260+
}
261+
c.ArgumentClassificationErrors["Component reference processing"] = err
262+
263+
return false
264+
}
265+
266+
func (c *AppConfig) addTemplateArguments(s string) bool {
267+
rc, err := app.IsPossibleTemplateFile(s)
268+
if rc {
269+
glog.V(2).Infof("treating %s as possible template file\n", s)
270+
c.Components = append(c.Components, s)
271+
return true
272+
}
273+
if err != nil {
274+
c.ArgumentClassificationErrors["Template processing"] = err
275+
}
276+
return false
277+
}
278+
222279
// AddArguments converts command line arguments into the appropriate bucket based on what they look like
223280
func (c *AppConfig) AddArguments(args []string) []string {
224281
unknown := []string{}
282+
c.ArgumentClassificationErrors = map[string]error{}
225283
for _, s := range args {
284+
if len(s) == 0 {
285+
continue
286+
}
287+
226288
switch {
227-
case cmdutil.IsEnvironmentArgument(s):
228-
glog.V(2).Infof("treating %s as possible environment argument\n", s)
229-
c.Environment = append(c.Environment, s)
230-
case app.IsPossibleSourceRepository(s):
231-
glog.V(2).Infof("treating %s as possible source repo\n", s)
232-
c.SourceRepositories = append(c.SourceRepositories, s)
233-
case app.IsComponentReference(s):
234-
glog.V(2).Infof("treating %s as a component ref\n", s)
235-
c.Components = append(c.Components, s)
236-
case app.IsPossibleTemplateFile(s):
237-
glog.V(2).Infof("treating %s as possible template file\n", s)
238-
c.Components = append(c.Components, s)
289+
case c.addEnvironmentArguments(s):
290+
case c.addSourceArguments(s):
291+
case c.addComponentArguments(s):
292+
case c.addTemplateArguments(s):
239293
default:
240294
glog.V(2).Infof("treating %s as unknown\n", s)
241-
if len(s) == 0 {
242-
break
243-
}
244295
unknown = append(unknown, s)
245296
}
297+
246298
}
247299
return unknown
248300
}
@@ -630,7 +682,7 @@ func (c *AppConfig) Run() (*AppResult, error) {
630682
// TODO: I don't belong here
631683
c.ensureDockerSearch()
632684

633-
resolved, err := Resolve(&c.Resolvers, &c.ComponentInputs, &c.GenerationInputs)
685+
resolved, err := Resolve(c)
634686
if err != nil {
635687
return nil, err
636688
}

‎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(appConfig)
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("when --strategy is specified you must provide at least one source code location; see the argument classification results above for possible explanations a source code location was not found")
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 %s provided to component reference check", s)
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: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@ os::cmd::expect_success_and_text 'oc new-app mysql -o yaml --as-test' 'test: tru
3838
# docker strategy with repo that has no dockerfile
3939
os::cmd::expect_failure_and_text 'oc new-app https://github.com/openshift/nodejs-ex --strategy=docker' 'No Dockerfile was found'
4040

41+
# repo related error message validation
42+
os::cmd::expect_failure_and_text 'oc new-app --strategy=docker https://192.30.253.113/openshift/ruby-hello-world.git' 'Remote access: '
43+
os::cmd::expect_failure_and_text 'oc new-app https://www.google.com/openshift/nodejs-e' 'Remote access: '
44+
os::cmd::expect_failure_and_text 'oc new-app https://githb.com/openshift/nodejs-e' 'Remote access: '
45+
os::cmd::expect_failure_and_text 'oc new-build --strategy=docker https://192.30.253.113/openshift/ruby-hello-world.git' 'Remote access: '
46+
os::cmd::expect_failure_and_text 'oc new-build https://www.google.com/openshift/nodejs-e' 'Remote access: '
47+
os::cmd::expect_failure_and_text 'oc new-build https://githb.com/openshift/nodejs-e' 'Remote access: '
48+
4149
# check label creation
4250
os::cmd::try_until_success 'oc get imagestreamtags php:latest'
4351
os::cmd::try_until_success 'oc get imagestreamtags php:5.5'

0 commit comments

Comments
 (0)