Skip to content
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

Jump to bottom

Adding a better error message for download mode #1492

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Adding a better error message for download mode #1492

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0224e53
Adding a better error message for download mode
arschles Dec 12, 2019
fa2631c
Fixing test
arschles Dec 12, 2019
834eba4
Making some progress toward removing the specific config error
arschles Dec 13, 2019
f45a03b
Fixing last test compile error
arschles Dec 13, 2019
4715103
Merge branch 'master' into config-msgs
arschles Dec 19, 2019
e928a0f
Merge branch 'master' into config-msgs
arschles Feb 14, 2020
32e7c28
Moving downloadModeURL to package-global
arschles Feb 14, 2020
a846557
using a valid HTTP error code instead of -1
arschles Feb 14, 2020
25b5860
removing the config.Validate function
arschles Feb 14, 2020
36b6d62
making the DownloadMode field required
arschles Feb 14, 2020
2c6b90b
validating that the port starts with ':'
arschles Feb 14, 2020
5b85142
Merge branch 'master' into config-msgs
arschles Feb 21, 2020
65662f5
Not passing an op to Validate functions
arschles Feb 21, 2020
e962a4d
Not checking for error strings in mode test
arschles Feb 21, 2020
1b29dcf
more doc for Validator
arschles Feb 21, 2020
d6a7cbd
removing struct tag for config port validation
arschles Feb 21, 2020
de71cb7
Merge branch 'master' into config-msgs
arschles Mar 10, 2020
cc70e12
Removing the unnecessary validator interface
arschles Mar 10, 2020
a5eda16
Joining error strings with newline+tabs instead of just a space
arschles Mar 10, 2020
198d574
s/retFile/df
arschles Mar 10, 2020
ee8f4d1
validating each download path in DownloadFile.Validate
arschles Mar 10, 2020
2cc9f84
Checking for custom or file modes
arschles Mar 10, 2020
9695c36
Checking top-level DownloadURL
arschles Mar 10, 2020
726b77c
Merge branch 'config-msgs' of github.com:arschles/athens into config-…
arschles Mar 10, 2020
4320101
validating the download path mode only in DownloadPath.Validate
arschles Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions cmd/proxy/actions/app.go
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ import (
"os"

"github.com/gomods/athens/pkg/config"
"github.com/gomods/athens/pkg/errors"
"github.com/gomods/athens/pkg/log"
mw "github.com/gomods/athens/pkg/middleware"
"github.com/gomods/athens/pkg/module"
Expand Down Expand Up @@ -132,6 +133,9 @@ func App(conf *config.Config) (http.Handler, error) {
lggr,
conf,
); err != nil {
if errors.IsConfigErr(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

so all that we're doing here is choosing not to wrap the error with "error adding proxy routes (message)". I think, instead of doing that, we can just pick a better wrapper-message and then we don't need this exposed API at all, we can just say: "error initializing Athens: %s" below.

return nil, err
}
err = fmt.Errorf("error adding proxy routes (%s)", err)
return nil, err
}
Expand Down
6 changes: 5 additions & 1 deletion cmd/proxy/actions/app_proxy.go
View file
Open in desktop
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/gomods/athens/pkg/download"
"github.com/gomods/athens/pkg/download/addons"
"github.com/gomods/athens/pkg/download/mode"
"github.com/gomods/athens/pkg/errors"
"github.com/gomods/athens/pkg/log"
"github.com/gomods/athens/pkg/module"
"github.com/gomods/athens/pkg/stash"
Expand Down Expand Up @@ -98,7 +99,10 @@ func addProxyRoutes(
}
st := stash.New(mf, s, stash.WithPool(c.GoGetWorkers), withSingleFlight)

df, err := mode.NewFile(c.DownloadMode, c.DownloadURL)
df, err := mode.NewFile(c.DownloadMode, c.DownloadURL, errors.ConfigError(
Copy link
Contributor

Choose a reason for hiding this comment

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

though this functional pattern is pretty awesome, it's a bit of an overkill just to add 2 lines of text to an error:

  1. We never have to customize this line
  2. We can just add those two lines into the errors themselves inside mod.NewFIle
  3. We can still keep it functional by keeping the mode.NewFile signature exactly the same, but having this functional pattern internal and private to the package itself so that you don't have to manually do it a few times

"DownloadMode",
"https://docs.gomods.io/configuration/download/",
))
if err != nil {
return err
}
Expand Down
18 changes: 8 additions & 10 deletions pkg/download/mode/mode.go
View file
Open in desktop
Expand Up @@ -18,13 +18,11 @@ type Mode string

// DownloadMode constants. For more information see config.dev.toml
const (
Sync Mode = "sync"
Async Mode = "async"
Redirect Mode = "redirect"
AsyncRedirect Mode = "async_redirect"
None Mode = "none"
downloadModeErr = "download mode is not set"
invalidModeErr = "unrecognized download mode: %s"
Sync Mode = "sync"
Async Mode = "async"
Redirect Mode = "redirect"
AsyncRedirect Mode = "async_redirect"
None Mode = "none"
)

// DownloadFile represents a custom HCL format of
Expand All @@ -50,11 +48,11 @@ type DownloadPath struct {
// you can either point to a file path by passing
// file:/path/to/file OR custom:<base64-encoded-hcl>
// directly.
func NewFile(m Mode, downloadURL string) (*DownloadFile, error) {
func NewFile(m Mode, downloadURL string, errFn errors.ConfigErrFn) (*DownloadFile, error) {
const op errors.Op = "downloadMode.NewFile"

if m == "" {
return nil, errors.E(op, downloadModeErr)
return nil, errFn("You have to pass a value.")
}

if strings.HasPrefix(string(m), "file:") {
Expand All @@ -76,7 +74,7 @@ func NewFile(m Mode, downloadURL string) (*DownloadFile, error) {
case Sync, Async, Redirect, AsyncRedirect, None:
return &DownloadFile{Mode: m, DownloadURL: downloadURL}, nil
default:
return nil, errors.E(op, errors.KindBadRequest, fmt.Sprintf(invalidModeErr, m))
return nil, errFn(fmt.Sprintf("%s isn't a valid value.", m))
}
}

Expand Down
12 changes: 9 additions & 3 deletions pkg/download/mode/mode_test.go
View file
Open in desktop
Expand Up @@ -3,6 +3,8 @@ package mode
import (
"fmt"
"testing"

"github.com/gomods/athens/pkg/errors"
)

var testCases = []struct {
Expand Down Expand Up @@ -119,6 +121,10 @@ func TestMode(t *testing.T) {
}

func TestNewFile_err(t *testing.T) {
dlModeErrFn := errors.ConfigError(
"DownloadMode",
"https://docs.gomods.io/configuration/download/",
)
tc := []struct {
name string
mode Mode
Expand All @@ -127,17 +133,17 @@ func TestNewFile_err(t *testing.T) {
{
name: "empty mode",
mode: "",
expected: downloadModeErr,
expected: dlModeErrFn("You have to pass a value.").Error(),
},
{
name: "invalid mode",
mode: "invalidMode",
expected: fmt.Sprintf(invalidModeErr, "invalidMode"),
expected: dlModeErrFn(fmt.Sprintf("%s isn't a valid value.", "invalidMode")).Error(),
},
}
for _, c := range tc {
t.Run(c.name, func(subT *testing.T) {
_, err := NewFile(c.mode, "github.com/gomods/athens")
_, err := NewFile(c.mode, "github.com/gomods/athens", dlModeErrFn)
if err.Error() != c.expected {
t.Fatalf("expected error %s from NewFile, got %s", c.expected, err.Error())
}
Expand Down
41 changes: 41 additions & 0 deletions pkg/errors/config.go
View file
Open in desktop
@@ -0,0 +1,41 @@
package errors

import (
"fmt"
"strings"
)

// ConfigErrFn is a function that can create a new configuration error. You pass it a
// message specific to the error you found when you were validating configuration,
// and it knows how to print out the actual configuration name and other helpful information.
type ConfigErrFn func(string) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here since this is specifically a ConfigErrFn, we can probably just put it in the Config package itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

@marwan-at-work I wanted to but can't because the config package imports mode which needs all the config error stuff. Do you think it should be somewhere besides there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant the mode package...which makes me realize ConfigErrFn makes no sense here, since it's not a Config package error, it's a mode error :)

So that said, all of my suggestion are meant for keeping this functionality local to wherever the error is happening, in this case inside mode.go

Thanks @arschles ✌️


type configErr struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a new error type? I think we can leverage the existing one. And instead of making a new error type, we can just make a new error Kind like ConfigErrorKind or something.

str string
}

func (c configErr) Error() string {
return c.str
}

// ConfigError returns a function that creates a configuration error to be printed
// to the terminal. Call this function and pass its return value down the call stack to
// functions that validate configuration fields.
//
// The function that ConfigError returns
//
// For example:
//
// downloadModeFn := ConfigError("DownloadMode (ATHENS_DOWNLOAD_MODE)")
// err := doThingsWithDownloadMode(configStruct, downloadModeFn)
func ConfigError(field string, url string) ConfigErrFn {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is pretty cool and functional :) but we can probably move this into the config package, because nobody else is leveraging it and so no need to increase the API surface of the errors package, we can just use the already exposed functionality.

return func(helpText string) error {
slc := []string{
fmt.Sprintf("There was a configuration error with %s. %s", field, helpText),
}
if url != "" {
slc = append(slc, fmt.Sprintf("See %s for more information.", url))
}
return &configErr{str: strings.Join(slc, "\n")}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just return errors.E(op, strings.Join(slc, "\n"), whateverKindIfNeedBe)

}
}
6 changes: 6 additions & 0 deletions pkg/errors/kinds.go
View file
Open in desktop
Expand Up @@ -4,3 +4,9 @@ package errors
func IsNotFoundErr(err error) bool {
return Kind(err) == KindNotFound
}

// IsConfigErr returns true if the given err is a configuration error
func IsConfigErr(err error) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably don't need this if we just do errors.Is(err, errors.ConfigErrorKind) or whatever kind name we'll eventually pick.

But furthermore, I don't think we need a new config error kind, because we don't programmatically do anything about it anyway, we just log the error and exit.

_, ok := err.(*configErr)
return ok
}