Skip to content

Commit

Permalink
Merge pull request #18272 from gabemontero/new-app-bld-msgs
Browse files Browse the repository at this point in the history 
Automatic merge from submit-queue (batch tested with PRs 18454, 18504, 18510, 18481, 18272).

adjust newapp/newbuild error messages (arg classification vs. actual …

…processing

Fixes #17925

@openshift/sig-developer-experience ptal

Now produces:

```
gmontero ~/go/src/github.com/openshift/origin  (new-app-bld-msgs)$ oc new-build --name imagesourcetest python~https://github.com/openshift-katacoda/blog-django-py --source-image xxx --source-image-path=yyy --dry-run
error: unable to locate resource for "xxx"

The 'oc new-build' command will match arguments to the following types:

  1. Images tagged into image streams in the current project or the 'openshift' project
     - if you don't specify a tag, we'll add ':latest'
  2. Images in the Docker Hub, on remote registries, or on the local Docker engine
  3. Git repository URLs or local paths that point to Git repositories

--allow-missing-images can be used to force the use of an image that was not matched

See 'oc new-build -h' for examples.

```
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored Feb 8, 2018
2 parents ebb0635 + 2785d75 commit 953814c
Show file tree
Hide file tree
Showing 14 changed files with 241 additions and 60 deletions.
80 changes: 59 additions & 21 deletions pkg/oc/cli/cmd/newapp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,29 @@ func CompleteAppConfig(config *newcmd.AppConfig, f *clientcmd.Factory, c *cobra.

unknown := config.AddArguments(args)
if len(unknown) != 0 {
return kcmdutil.UsageErrorf(c, "Did not recognize the following arguments: %v", unknown)
buf := &bytes.Buffer{}
fmt.Fprintf(buf, "Did not recognize the following arguments: %v\n\n", unknown)
for _, argName := range unknown {
fmt.Fprintf(buf, "%s:\n", argName)
for _, classErr := range config.EnvironmentClassificationErrors {
if classErr.Value != nil {
fmt.Fprintf(buf, fmt.Sprintf("%s: %v\n", classErr.Key, classErr.Value))
} else {
fmt.Fprintf(buf, fmt.Sprintf("%s\n", classErr.Key))
}
}
for _, classErr := range config.SourceClassificationErrors {
fmt.Fprintf(buf, fmt.Sprintf("%s: %v\n", classErr.Key, classErr.Value))
}
for _, classErr := range config.TemplateClassificationErrors {
fmt.Fprintf(buf, fmt.Sprintf("%s: %v\n", classErr.Key, classErr.Value))
}
for _, classErr := range config.ComponentClassificationErrors {
fmt.Fprintf(buf, fmt.Sprintf("%s: %v\n", classErr.Key, classErr.Value))
}
fmt.Fprintln(buf)
}
return kcmdutil.UsageErrorf(c, heredoc.Docf(buf.String()))
}

if config.AllowMissingImages && config.AsSearch {
Expand Down Expand Up @@ -701,7 +723,7 @@ func retryBuildConfig(info *resource.Info, err error) runtime.Object {
return nil
}

func handleError(err error, baseName, commandName, commandPath string, config *newcmd.AppConfig, transformError func(err error, baseName, commandName, commandPath string, groups errorGroups)) error {
func handleError(err error, baseName, commandName, commandPath string, config *newcmd.AppConfig, transformError func(err error, baseName, commandName, commandPath string, groups errorGroups, config *newcmd.AppConfig)) error {
if err == nil {
return nil
}
Expand All @@ -711,23 +733,19 @@ func handleError(err error, baseName, commandName, commandPath string, config *n
}
groups := errorGroups{}
for _, err := range errs {
transformError(err, baseName, commandName, commandPath, groups)
transformError(err, baseName, commandName, commandPath, groups, config)
}
buf := &bytes.Buffer{}
if len(config.ArgumentClassificationErrors) > 0 {
fmt.Fprintf(buf, "Errors occurred while determining argument types:\n")
for _, classErr := range config.ArgumentClassificationErrors {
fmt.Fprintf(buf, fmt.Sprintf("\n%s: %v\n", classErr.Key, classErr.Value))
}
fmt.Fprint(buf, "\n")
// this print serves as a header for the printing of the errorGroups, but
// only print it if we precede with classification errors, to help distinguish
// between the two
fmt.Fprintln(buf, "Errors occurred during resource creation:")
}
for _, group := range groups {
fmt.Fprint(buf, kcmdutil.MultipleErrors("error: ", group.errs))
if len(group.classification) > 0 {
fmt.Fprintln(buf)
}
fmt.Fprintf(buf, group.classification)
if len(group.suggestion) > 0 {
if len(group.classification) > 0 {
fmt.Fprintln(buf)
}
fmt.Fprintln(buf)
}
fmt.Fprint(buf, group.suggestion)
Expand All @@ -736,20 +754,22 @@ func handleError(err error, baseName, commandName, commandPath string, config *n
}

type errorGroup struct {
errs []error
suggestion string
errs []error
suggestion string
classification string
}
type errorGroups map[string]errorGroup

func (g errorGroups) Add(group string, suggestion string, err error, errs ...error) {
func (g errorGroups) Add(group string, suggestion string, classification string, err error, errs ...error) {
all := g[group]
all.errs = append(all.errs, errs...)
all.errs = append(all.errs, err)
all.suggestion = suggestion
all.classification = classification
g[group] = all
}

func transformRunError(err error, baseName, commandName, commandPath string, groups errorGroups) {
func transformRunError(err error, baseName, commandName, commandPath string, groups errorGroups, config *newcmd.AppConfig) {
switch t := err.(type) {
case newcmd.ErrRequiresExplicitAccess:
if t.Input.Token != nil && t.Input.Token.ServiceAccount {
Expand All @@ -762,6 +782,7 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
You can see more information about the image by adding the --dry-run flag.
If you trust the provided image, include the flag --grant-install-rights.`,
),
"",
fmt.Errorf("installing %q requires an 'installer' service account with project editor access", t.Match.Value),
)
} else {
Expand All @@ -774,11 +795,19 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
You can see more information about the image by adding the --dry-run flag.
If you trust the provided image, include the flag --grant-install-rights.`,
),
"",
fmt.Errorf("installing %q requires that you grant the image access to run with your credentials", t.Match.Value),
)
}
return
case newapp.ErrNoMatch:
classification, _ := config.ClassificationWinners[t.Value]
if classification.IncludeGitErrors {
notGitRepo, ok := config.SourceClassificationErrors[t.Value]
if ok {
t.Errs = append(t.Errs, notGitRepo.Value)
}
}
groups.Add(
"no-matches",
heredoc.Docf(`
Expand All @@ -794,11 +823,13 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
See '%[1]s -h' for examples.`, commandPath,
),
heredoc.Docf(classification.String()),
t,
t.Errs...,
)
return
case newapp.ErrMultipleMatches:
classification, _ := config.ClassificationWinners[t.Value]
buf := &bytes.Buffer{}
for i, match := range t.Matches {

Expand All @@ -812,6 +843,7 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
%[2]sTo view a full list of matches, use '%[3]s %[4]s -S %[1]s'`, t.Value, buf.String(), baseName, commandName,
),
classification.String(),
t,
t.Errs...,
)
Expand All @@ -830,11 +862,13 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
%[2]s`, t.Value, buf.String(),
),
classification.String(),
t,
t.Errs...,
)
return
case newapp.ErrPartialMatch:
classification, _ := config.ClassificationWinners[t.Value]
buf := &bytes.Buffer{}
fmt.Fprintf(buf, "* %s\n", t.Match.Description)
fmt.Fprintf(buf, " Use %[1]s to specify this image or template\n\n", t.Match.Argument)
Expand All @@ -846,11 +880,13 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
%[2]s`, t.Value, buf.String(),
),
classification.String(),
t,
t.Errs...,
)
return
case newapp.ErrNoTagsFound:
classification, _ := config.ClassificationWinners[t.Value]
buf := &bytes.Buffer{}
fmt.Fprintf(buf, " Use --allow-missing-imagestream-tags to use this image stream\n\n")
groups.Add(
Expand All @@ -860,6 +896,7 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
%[2]s`, t.Match.Name, buf.String(),
),
classification.String(),
t,
t.Errs...,
)
Expand All @@ -868,13 +905,14 @@ func transformRunError(err error, baseName, commandName, commandPath string, gro
switch err {
case errNoTokenAvailable:
// TODO: improve by allowing token generation
groups.Add("", "", fmt.Errorf("to install components you must be logged in with an OAuth token (instead of only a certificate)"))
groups.Add("", "", "", fmt.Errorf("to install components you must be logged in with an OAuth token (instead of only a certificate)"))
case newcmd.ErrNoInputs:
// TODO: suggest things to the user
groups.Add("", "", usageError(commandPath, newAppNoInput, baseName, commandName))
groups.Add("", "", "", usageError(commandPath, newAppNoInput, baseName, commandName))
default:
groups.Add("", "", err)
groups.Add("", "", "", err)
}
return
}

func usageError(commandPath, format string, args ...interface{}) error {
Expand Down
15 changes: 12 additions & 3 deletions pkg/oc/cli/cmd/newbuild.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,16 @@ func (o *NewBuildOptions) RunNewBuild() error {
return nil
}

func transformBuildError(err error, baseName, commandName, commandPath string, groups errorGroups) {
func transformBuildError(err error, baseName, commandName, commandPath string, groups errorGroups, config *newcmd.AppConfig) {
switch t := err.(type) {
case newapp.ErrNoMatch:
classification, _ := config.ClassificationWinners[t.Value]
if classification.IncludeGitErrors {
notGitRepo, ok := config.SourceClassificationErrors[t.Value]
if ok {
t.Errs = append(t.Errs, notGitRepo.Value)
}
}
groups.Add(
"no-matches",
heredoc.Docf(`
Expand All @@ -230,15 +237,17 @@ func transformBuildError(err error, baseName, commandName, commandPath string, g
See '%[1]s -h' for examples.`, commandPath,
),
classification.String(),
t,
t.Errs...,
)
return
}
switch err {
case newcmd.ErrNoInputs:
groups.Add("", "", usageError(commandPath, newBuildNoInput, baseName, commandName))
groups.Add("", "", "", usageError(commandPath, newBuildNoInput, baseName, commandName))
return
}
transformRunError(err, baseName, commandName, commandPath, groups)
transformRunError(err, baseName, commandName, commandPath, groups, config)
return
}
4 changes: 4 additions & 0 deletions pkg/oc/cli/cmd/newbuild_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ type MockSearcher struct {
OnSearch func(precise bool, terms ...string) (app.ComponentMatches, []error)
}

func (m MockSearcher) Type() string {
return ""
}

// Search mocks a search.
func (m MockSearcher) Search(precise bool, terms ...string) (app.ComponentMatches, []error) {
return m.OnSearch(precise, terms...)
Expand Down
30 changes: 25 additions & 5 deletions pkg/oc/generate/app/componentresolvers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package app

import (
"sort"
"strings"

"github.com/golang/glog"

Expand All @@ -24,6 +25,7 @@ type Resolver interface {
// matches
type Searcher interface {
Search(precise bool, terms ...string) (ComponentMatches, []error)
Type() string
}

// WeightedResolver is a resolver identified as exact or not, depending on its weight
Expand All @@ -42,6 +44,7 @@ type PerfectMatchWeightedResolver []WeightedResolver
// Resolve resolves the provided input and returns only exact matches
func (r PerfectMatchWeightedResolver) Resolve(value string) (*ComponentMatch, error) {
var errs []error
types := []string{}
candidates := ScoredComponentMatches{}
var group MultiSimpleSearcher
var groupWeight float32 = 0.0
Expand All @@ -59,6 +62,7 @@ func (r PerfectMatchWeightedResolver) Resolve(value string) (*ComponentMatch, er
glog.V(5).Infof("Error from resolver: %v\n", err)
errs = append(errs, err...)
}
types = append(types, group.Type())

sort.Sort(ScoredComponentMatches(matches))
if len(matches) > 0 && matches[0].Score == 0.0 && (len(matches) == 1 || matches[1].Score != 0.0) {
Expand All @@ -75,7 +79,7 @@ func (r PerfectMatchWeightedResolver) Resolve(value string) (*ComponentMatch, er

switch len(candidates) {
case 0:
return nil, ErrNoMatch{Value: value, Errs: errs}
return nil, ErrNoMatch{Value: value, Errs: errs, Type: strings.Join(types, ", ")}
case 1:
if candidates[0].Score != 0.0 {
if candidates[0].NoTagsFound {
Expand Down Expand Up @@ -108,7 +112,7 @@ type FirstMatchResolver struct {
func (r FirstMatchResolver) Resolve(value string) (*ComponentMatch, error) {
matches, err := r.Searcher.Search(true, value)
if len(matches) == 0 {
return nil, ErrNoMatch{Value: value, Errs: err}
return nil, ErrNoMatch{Value: value, Errs: err, Type: r.Searcher.Type()}
}
return matches[0], errors.NewAggregate(err)
}
Expand All @@ -125,7 +129,7 @@ type HighestScoreResolver struct {
func (r HighestScoreResolver) Resolve(value string) (*ComponentMatch, error) {
matches, err := r.Searcher.Search(true, value)
if len(matches) == 0 {
return nil, ErrNoMatch{Value: value, Errs: err}
return nil, ErrNoMatch{Value: value, Errs: err, Type: r.Searcher.Type()}
}
sort.Sort(ScoredComponentMatches(matches))
return matches[0], errors.NewAggregate(err)
Expand All @@ -146,7 +150,7 @@ func (r HighestUniqueScoreResolver) Resolve(value string) (*ComponentMatch, erro
sort.Sort(ScoredComponentMatches(matches))
switch len(matches) {
case 0:
return nil, ErrNoMatch{Value: value, Errs: err}
return nil, ErrNoMatch{Value: value, Errs: err, Type: r.Searcher.Type()}
case 1:
return matches[0], errors.NewAggregate(err)
default:
Expand Down Expand Up @@ -184,7 +188,7 @@ func (r UniqueExactOrInexactMatchResolver) Resolve(value string) (*ComponentMatc
inexact := matches.Inexact()
switch len(inexact) {
case 0:
return nil, ErrNoMatch{Value: value, Errs: err}
return nil, ErrNoMatch{Value: value, Errs: err, Type: r.Searcher.Type()}
case 1:
return inexact[0], errors.NewAggregate(err)
default:
Expand Down Expand Up @@ -213,6 +217,14 @@ func (r PipelineResolver) Resolve(value string) (*ComponentMatch, error) {
// MultiSimpleSearcher is a set of searchers
type MultiSimpleSearcher []Searcher

func (s MultiSimpleSearcher) Type() string {
t := []string{}
for _, searcher := range s {
t = append(t, searcher.Type())
}
return strings.Join(t, ", ")
}

// Search searches using all searchers it holds
func (s MultiSimpleSearcher) Search(precise bool, terms ...string) (ComponentMatches, []error) {
var errs []error
Expand All @@ -239,6 +251,14 @@ type WeightedSearcher struct {
// priority in search results
type MultiWeightedSearcher []WeightedSearcher

func (s MultiWeightedSearcher) Type() string {
t := []string{}
for _, searcher := range s {
t = append(t, searcher.Type())
}
return strings.Join(t, ", ")
}

// Search searches using all searchers it holds and score according to searcher height
func (s MultiWeightedSearcher) Search(precise bool, terms ...string) (ComponentMatches, []error) {
componentMatches := ComponentMatches{}
Expand Down
4 changes: 4 additions & 0 deletions pkg/oc/generate/app/componentresolvers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ type mockSearcher struct {
numResults int
}

func (m mockSearcher) Type() string {
return ""
}

func (m mockSearcher) Search(precise bool, terms ...string) (ComponentMatches, []error) {
results := ComponentMatches{}
for i := 0; i < m.numResults; i++ {
Expand Down
Loading

0 comments on commit 953814c

Please sign in to comment.