From 60e54a1368da255addb0ead6defdf69cfc402e22 Mon Sep 17 00:00:00 2001 From: Carlos Alexandro Becker Date: Tue, 22 Jan 2019 01:56:16 -0200 Subject: [PATCH] refactor/fix: improved CLI (#937) * refactor: added middleware for action logs/error handling * refactor: moved custom changelog load from main.go * fix/refactor: CLI improvements * test: do not pollute ./dist --- internal/middleware/doc.go | 2 + internal/middleware/error.go | 23 ++++++++ internal/middleware/error_test.go | 23 ++++++++ internal/middleware/logging.go | 37 ++++++++++++ internal/middleware/logging_test.go | 11 ++++ internal/middleware/middleware.go | 8 +++ internal/middleware/middleware_test.go | 11 ++++ internal/pipe/changelog/changelog.go | 20 ++++++- internal/pipe/changelog/changelog_test.go | 63 ++++++++++++--------- internal/pipe/changelog/testdata/changes.md | 1 + internal/pipe/defaults/defaults.go | 9 ++- internal/pipe/publish/publish.go | 23 ++------ main.go | 58 +++++-------------- main_test.go | 5 +- pkg/context/context.go | 1 - 15 files changed, 200 insertions(+), 95 deletions(-) create mode 100644 internal/middleware/doc.go create mode 100644 internal/middleware/error.go create mode 100644 internal/middleware/error_test.go create mode 100644 internal/middleware/logging.go create mode 100644 internal/middleware/logging_test.go create mode 100644 internal/middleware/middleware.go create mode 100644 internal/middleware/middleware_test.go create mode 100644 internal/pipe/changelog/testdata/changes.md diff --git a/internal/middleware/doc.go b/internal/middleware/doc.go new file mode 100644 index 000000000..9ab9c5f45 --- /dev/null +++ b/internal/middleware/doc.go @@ -0,0 +1,2 @@ +// Package middleware define middlewares for Actions. +package middleware diff --git a/internal/middleware/error.go b/internal/middleware/error.go new file mode 100644 index 000000000..6cdf2cf32 --- /dev/null +++ b/internal/middleware/error.go @@ -0,0 +1,23 @@ +package middleware + +import ( + "github.com/apex/log" + "github.com/goreleaser/goreleaser/internal/pipe" + "github.com/goreleaser/goreleaser/pkg/context" +) + +// ErrHandler handles an action error, ignoring and logging pipe skipped +// errors. +func ErrHandler(action Action) Action { + return func(ctx *context.Context) error { + var err = action(ctx) + if err == nil { + return nil + } + if pipe.IsSkip(err) { + log.WithError(err).Warn("pipe skipped") + return nil + } + return err + } +} diff --git a/internal/middleware/error_test.go b/internal/middleware/error_test.go new file mode 100644 index 000000000..a92582d9e --- /dev/null +++ b/internal/middleware/error_test.go @@ -0,0 +1,23 @@ +package middleware + +import ( + "fmt" + "testing" + + "github.com/goreleaser/goreleaser/internal/pipe" + "github.com/stretchr/testify/require" +) + +func TestError(t *testing.T) { + t.Run("no errors", func(t *testing.T) { + require.NoError(t, ErrHandler(mockAction(nil))(ctx)) + }) + + t.Run("pipe skipped", func(t *testing.T) { + require.NoError(t, ErrHandler(mockAction(pipe.ErrSkipValidateEnabled))(ctx)) + }) + + t.Run("some err", func(t *testing.T) { + require.Error(t, ErrHandler(mockAction(fmt.Errorf("pipe errored")))(ctx)) + }) +} diff --git a/internal/middleware/logging.go b/internal/middleware/logging.go new file mode 100644 index 000000000..34315a5ff --- /dev/null +++ b/internal/middleware/logging.go @@ -0,0 +1,37 @@ +package middleware + +import ( + "strings" + + "github.com/apex/log" + "github.com/apex/log/handlers/cli" + "github.com/fatih/color" + "github.com/goreleaser/goreleaser/pkg/context" +) + +// Padding is a logging initial padding. +type Padding int + +// DefaultInitialPadding is the default padding in the log library. +const DefaultInitialPadding Padding = 3 + +// ExtraPadding is the double of the DefaultInitialPadding. +const ExtraPadding Padding = DefaultInitialPadding * 2 + +// Logging pretty prints the given action and its title. +// You can have different padding levels by providing different initial +// paddings. The middleware will print the title in the given padding and the +// action logs in padding+default padding. +// The default padding in the log library is 3. +// The middleware always resets to the default padding. +func Logging(title string, next Action, padding Padding) Action { + return func(ctx *context.Context) error { + defer func() { + cli.Default.Padding = int(DefaultInitialPadding) + }() + cli.Default.Padding = int(padding) + log.Infof(color.New(color.Bold).Sprint(strings.ToUpper(title))) + cli.Default.Padding = int(padding + DefaultInitialPadding) + return next(ctx) + } +} diff --git a/internal/middleware/logging_test.go b/internal/middleware/logging_test.go new file mode 100644 index 000000000..8d9dfb516 --- /dev/null +++ b/internal/middleware/logging_test.go @@ -0,0 +1,11 @@ +package middleware + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestLogging(t *testing.T) { + require.NoError(t, Logging("foo", mockAction(nil), DefaultInitialPadding)(ctx)) +} diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go new file mode 100644 index 000000000..de81de7d1 --- /dev/null +++ b/internal/middleware/middleware.go @@ -0,0 +1,8 @@ +package middleware + +import "github.com/goreleaser/goreleaser/pkg/context" + +// Action is a function that takes a context and returns an error. +// It is is used on Pipers, Defaulters and Publishers, although they are not +// aware of this generalization. +type Action func(ctx *context.Context) error diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go new file mode 100644 index 000000000..a47e763a7 --- /dev/null +++ b/internal/middleware/middleware_test.go @@ -0,0 +1,11 @@ +package middleware + +import "github.com/goreleaser/goreleaser/pkg/context" + +var ctx = &context.Context{} + +func mockAction(err error) Action { + return func(ctx *context.Context) error { + return err + } +} diff --git a/internal/pipe/changelog/changelog.go b/internal/pipe/changelog/changelog.go index a32c1d9a8..ee97a3560 100644 --- a/internal/pipe/changelog/changelog.go +++ b/internal/pipe/changelog/changelog.go @@ -11,7 +11,6 @@ import ( "strings" "github.com/apex/log" - "github.com/goreleaser/goreleaser/internal/git" "github.com/goreleaser/goreleaser/internal/pipe" "github.com/goreleaser/goreleaser/pkg/context" @@ -32,8 +31,15 @@ func (Pipe) Run(ctx *context.Context) error { if ctx.Config.Changelog.Skip { return pipe.Skip("changelog should not be built") } + // TODO: should probably have a different field for the filename and its + // contents. if ctx.ReleaseNotes != "" { - return pipe.Skip("release notes already provided via --release-notes") + notes, err := loadFromFile(ctx.ReleaseNotes) + if err != nil { + return err + } + ctx.ReleaseNotes = notes + return nil } if ctx.Snapshot { return pipe.Skip("not available for snapshots") @@ -51,6 +57,16 @@ func (Pipe) Run(ctx *context.Context) error { return ioutil.WriteFile(path, []byte(ctx.ReleaseNotes), 0644) } +func loadFromFile(file string) (string, error) { + bts, err := ioutil.ReadFile(file) + if err != nil { + return "", err + } + log.WithField("file", file).Info("loaded custom release notes") + log.WithField("file", file).Debugf("custom release notes: \n%s", string(bts)) + return string(bts), nil +} + func checkSortDirection(mode string) error { switch mode { case "": diff --git a/internal/pipe/changelog/changelog_test.go b/internal/pipe/changelog/changelog_test.go index 0563787cc..eb86dad11 100644 --- a/internal/pipe/changelog/changelog_test.go +++ b/internal/pipe/changelog/changelog_test.go @@ -5,7 +5,7 @@ import ( "path/filepath" "testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/goreleaser/goreleaser/internal/testlib" "github.com/goreleaser/goreleaser/pkg/config" @@ -13,13 +13,20 @@ import ( ) func TestDescription(t *testing.T) { - assert.NotEmpty(t, Pipe{}.String()) + require.NotEmpty(t, Pipe{}.String()) } func TestChangelogProvidedViaFlag(t *testing.T) { var ctx = context.New(config.Project{}) - ctx.ReleaseNotes = "c0ff33 foo bar" - testlib.AssertSkipped(t, Pipe{}.Run(ctx)) + ctx.ReleaseNotes = "testdata/changes.md" + require.NoError(t, Pipe{}.Run(ctx)) + require.Equal(t, "c0ff33 coffeee\n", ctx.ReleaseNotes) +} + +func TestChangelogProvidedViaFlagDoesntExist(t *testing.T) { + var ctx = context.New(config.Project{}) + ctx.ReleaseNotes = "testdata/changes.nope" + require.EqualError(t, Pipe{}.Run(ctx), "open testdata/changes.nope: no such file or directory") } func TestChangelogSkip(t *testing.T) { @@ -63,19 +70,19 @@ func TestChangelog(t *testing.T) { }, }) ctx.Git.CurrentTag = "v0.0.2" - assert.NoError(t, Pipe{}.Run(ctx)) - assert.Contains(t, ctx.ReleaseNotes, "## Changelog") - assert.NotContains(t, ctx.ReleaseNotes, "first") - assert.Contains(t, ctx.ReleaseNotes, "added feature 1") - assert.Contains(t, ctx.ReleaseNotes, "fixed bug 2") - assert.NotContains(t, ctx.ReleaseNotes, "docs") - assert.NotContains(t, ctx.ReleaseNotes, "ignored") - assert.NotContains(t, ctx.ReleaseNotes, "cArs") - assert.NotContains(t, ctx.ReleaseNotes, "from goreleaser/some-branch") + require.NoError(t, Pipe{}.Run(ctx)) + require.Contains(t, ctx.ReleaseNotes, "## Changelog") + require.NotContains(t, ctx.ReleaseNotes, "first") + require.Contains(t, ctx.ReleaseNotes, "added feature 1") + require.Contains(t, ctx.ReleaseNotes, "fixed bug 2") + require.NotContains(t, ctx.ReleaseNotes, "docs") + require.NotContains(t, ctx.ReleaseNotes, "ignored") + require.NotContains(t, ctx.ReleaseNotes, "cArs") + require.NotContains(t, ctx.ReleaseNotes, "from goreleaser/some-branch") bts, err := ioutil.ReadFile(filepath.Join(folder, "CHANGELOG.md")) - assert.NoError(t, err) - assert.NotEmpty(t, string(bts)) + require.NoError(t, err) + require.NotEmpty(t, string(bts)) } func TestChangelogSort(t *testing.T) { @@ -125,14 +132,14 @@ func TestChangelogSort(t *testing.T) { t.Run("changelog sort='"+cfg.Sort+"'", func(t *testing.T) { ctx.Config.Changelog.Sort = cfg.Sort entries, err := buildChangelog(ctx) - assert.NoError(t, err) - assert.Len(t, entries, len(cfg.Entries)) + require.NoError(t, err) + require.Len(t, entries, len(cfg.Entries)) var changes []string for _, line := range entries { _, msg := extractCommitInfo(line) changes = append(changes, msg) } - assert.EqualValues(t, cfg.Entries, changes) + require.EqualValues(t, cfg.Entries, changes) }) } } @@ -143,7 +150,7 @@ func TestChangelogInvalidSort(t *testing.T) { Sort: "dope", }, }) - assert.EqualError(t, Pipe{}.Run(ctx), ErrInvalidSortDirection.Error()) + require.EqualError(t, Pipe{}.Run(ctx), ErrInvalidSortDirection.Error()) } func TestChangelogOfFirstRelease(t *testing.T) { @@ -162,10 +169,10 @@ func TestChangelogOfFirstRelease(t *testing.T) { testlib.GitTag(t, "v0.0.1") var ctx = context.New(config.Project{}) ctx.Git.CurrentTag = "v0.0.1" - assert.NoError(t, Pipe{}.Run(ctx)) - assert.Contains(t, ctx.ReleaseNotes, "## Changelog") + require.NoError(t, Pipe{}.Run(ctx)) + require.Contains(t, ctx.ReleaseNotes, "## Changelog") for _, msg := range msgs { - assert.Contains(t, ctx.ReleaseNotes, msg) + require.Contains(t, ctx.ReleaseNotes, msg) } } @@ -187,7 +194,7 @@ func TestChangelogFilterInvalidRegex(t *testing.T) { }, }) ctx.Git.CurrentTag = "v0.0.4" - assert.EqualError(t, Pipe{}.Run(ctx), "error parsing regexp: invalid or unsupported Perl syntax: `(?ia`") + require.EqualError(t, Pipe{}.Run(ctx), "error parsing regexp: invalid or unsupported Perl syntax: `(?ia`") } func TestChangelogNoTags(t *testing.T) { @@ -196,8 +203,8 @@ func TestChangelogNoTags(t *testing.T) { testlib.GitInit(t) testlib.GitCommit(t, "first") var ctx = context.New(config.Project{}) - assert.Error(t, Pipe{}.Run(ctx)) - assert.Empty(t, ctx.ReleaseNotes) + require.Error(t, Pipe{}.Run(ctx)) + require.Empty(t, ctx.ReleaseNotes) } func TestChangelogOnBranchWithSameNameAsTag(t *testing.T) { @@ -217,9 +224,9 @@ func TestChangelogOnBranchWithSameNameAsTag(t *testing.T) { testlib.GitCheckoutBranch(t, "v0.0.1") var ctx = context.New(config.Project{}) ctx.Git.CurrentTag = "v0.0.1" - assert.NoError(t, Pipe{}.Run(ctx)) - assert.Contains(t, ctx.ReleaseNotes, "## Changelog") + require.NoError(t, Pipe{}.Run(ctx)) + require.Contains(t, ctx.ReleaseNotes, "## Changelog") for _, msg := range msgs { - assert.Contains(t, ctx.ReleaseNotes, msg) + require.Contains(t, ctx.ReleaseNotes, msg) } } diff --git a/internal/pipe/changelog/testdata/changes.md b/internal/pipe/changelog/testdata/changes.md new file mode 100644 index 000000000..cc9f88e3a --- /dev/null +++ b/internal/pipe/changelog/testdata/changes.md @@ -0,0 +1 @@ +c0ff33 coffeee diff --git a/internal/pipe/defaults/defaults.go b/internal/pipe/defaults/defaults.go index e1e91c9d0..62950c8c5 100644 --- a/internal/pipe/defaults/defaults.go +++ b/internal/pipe/defaults/defaults.go @@ -3,7 +3,7 @@ package defaults import ( - "github.com/apex/log" + "github.com/goreleaser/goreleaser/internal/middleware" "github.com/goreleaser/goreleaser/pkg/context" "github.com/goreleaser/goreleaser/pkg/defaults" ) @@ -24,8 +24,11 @@ func (Pipe) Run(ctx *context.Context) error { ctx.Config.GitHubURLs.Download = "https://github.com" } for _, defaulter := range defaults.Defaulters { - log.Debug(defaulter.String()) - if err := defaulter.Default(ctx); err != nil { + if err := middleware.Logging( + defaulter.String(), + middleware.ErrHandler(defaulter.Default), + middleware.ExtraPadding, + )(ctx); err != nil { return err } } diff --git a/internal/pipe/publish/publish.go b/internal/pipe/publish/publish.go index 9e59d136d..0ed8e556e 100644 --- a/internal/pipe/publish/publish.go +++ b/internal/pipe/publish/publish.go @@ -4,8 +4,7 @@ package publish import ( "fmt" - "github.com/apex/log" - "github.com/fatih/color" + "github.com/goreleaser/goreleaser/internal/middleware" "github.com/goreleaser/goreleaser/internal/pipe" "github.com/goreleaser/goreleaser/internal/pipe/artifactory" "github.com/goreleaser/goreleaser/internal/pipe/brew" @@ -54,23 +53,13 @@ func (Pipe) Run(ctx *context.Context) error { return pipe.ErrSkipPublishEnabled } for _, publisher := range publishers { - log.Infof(color.New(color.Bold).Sprint(publisher.String())) - if err := handle(publisher.Publish(ctx)); err != nil { + if err := middleware.Logging( + publisher.String(), + middleware.ErrHandler(publisher.Publish), + middleware.ExtraPadding, + )(ctx); err != nil { return errors.Wrapf(err, "%s: failed to publish artifacts", publisher.String()) } } return nil } - -// TODO: for now this is duplicated, we should have better error handling -// eventually. -func handle(err error) error { - if err == nil { - return nil - } - if pipe.IsSkip(err) { - log.WithField("reason", err.Error()).Warn("skipped") - return nil - } - return err -} diff --git a/main.go b/main.go index be07ec446..09b5f309d 100644 --- a/main.go +++ b/main.go @@ -4,14 +4,13 @@ import ( "fmt" "io/ioutil" "os" - "strings" "time" "github.com/apex/log" "github.com/apex/log/handlers/cli" "github.com/caarlos0/ctrlc" "github.com/fatih/color" - "github.com/goreleaser/goreleaser/internal/pipe" + "github.com/goreleaser/goreleaser/internal/middleware" "github.com/goreleaser/goreleaser/internal/pipeline" "github.com/goreleaser/goreleaser/internal/static" "github.com/goreleaser/goreleaser/pkg/config" @@ -34,7 +33,6 @@ type releaseOptions struct { SkipSign bool SkipValidate bool RmDist bool - Debug bool Parallelism int Timeout time.Duration } @@ -56,11 +54,11 @@ func main() { var config = releaseCmd.Flag("config", "Load configuration from file").Short('c').Short('f').PlaceHolder(".goreleaser.yml").String() var releaseNotes = releaseCmd.Flag("release-notes", "Load custom release notes from a markdown file").PlaceHolder("notes.md").String() var snapshot = releaseCmd.Flag("snapshot", "Generate an unversioned snapshot release, skipping all validations and without publishing any artifacts").Bool() - var skipPublish = releaseCmd.Flag("skip-publish", "Generates all artifacts but does not publish them anywhere").Bool() + var skipPublish = releaseCmd.Flag("skip-publish", "Skips publishing artifacts").Bool() var skipSign = releaseCmd.Flag("skip-sign", "Skips signing the artifacts").Bool() - var skipValidate = releaseCmd.Flag("skip-validate", "Skips all git sanity checks").Bool() + var skipValidate = releaseCmd.Flag("skip-validate", "Skips several sanity checks").Bool() var rmDist = releaseCmd.Flag("rm-dist", "Remove the dist folder before building").Bool() - var parallelism = releaseCmd.Flag("parallelism", "Amount of slow tasks to do in concurrently").Short('p').Default("4").Int() // TODO: use runtime.NumCPU here? + var parallelism = releaseCmd.Flag("parallelism", "Amount tasks to run concurrently").Short('p').Default("4").Int() var timeout = releaseCmd.Flag("timeout", "Timeout to the entire release process").Default("30m").Duration() app.Version(fmt.Sprintf("%v, commit %v, built at %v", version, commit, date)) @@ -93,7 +91,6 @@ func main() { SkipSign: *skipSign, RmDist: *rmDist, Parallelism: *parallelism, - Debug: *debug, Timeout: *timeout, } if err := releaseProject(options); err != nil { @@ -113,50 +110,25 @@ func releaseProject(options releaseOptions) error { ctx, cancel := context.NewWithTimeout(cfg, options.Timeout) defer cancel() ctx.Parallelism = options.Parallelism - ctx.Debug = options.Debug log.Debugf("parallelism: %v", ctx.Parallelism) - if options.ReleaseNotes != "" { - bts, err := ioutil.ReadFile(options.ReleaseNotes) - if err != nil { - return err - } - log.WithField("file", options.ReleaseNotes).Info("loaded custom release notes") - log.WithField("file", options.ReleaseNotes).Debugf("custom release notes: \n%s", string(bts)) - ctx.ReleaseNotes = string(bts) - } + ctx.ReleaseNotes = options.ReleaseNotes ctx.Snapshot = options.Snapshot ctx.SkipPublish = ctx.Snapshot || options.SkipPublish ctx.SkipValidate = ctx.Snapshot || options.SkipValidate ctx.SkipSign = options.SkipSign ctx.RmDist = options.RmDist - return doRelease(ctx) -} - -func doRelease(ctx *context.Context) error { - defer func() { cli.Default.Padding = 3 }() - var release = func() error { + return ctrlc.Default.Run(ctx, func() error { for _, pipe := range pipeline.Pipeline { - cli.Default.Padding = 3 - log.Infof(color.New(color.Bold).Sprint(strings.ToUpper(pipe.String()))) - cli.Default.Padding = 6 - if err := handle(pipe.Run(ctx)); err != nil { + if err := middleware.Logging( + pipe.String(), + middleware.ErrHandler(pipe.Run), + middleware.DefaultInitialPadding, + )(ctx); err != nil { return err } } return nil - } - return ctrlc.Default.Run(ctx, release) -} - -func handle(err error) error { - if err == nil { - return nil - } - if pipe.IsSkip(err) { - log.WithField("reason", err.Error()).Warn("skipped") - return nil - } - return err + }) } // InitProject creates an example goreleaser.yml in the current directory @@ -187,8 +159,8 @@ func loadConfig(path string) (config.Project, error) { } return proj, err } - // the user didn't specified a config file and the known files - // doest not exist, so, return an empty config and a nil err. - log.Warn("could not load config, using defaults") + // the user didn't specified a config file and the known possible file names + // don't exist, so, return an empty config and a nil err. + log.Warn("could find a config file, using defaults...") return config.Project{}, nil } diff --git a/main_test.go b/main_test.go index 29c50f473..b81ccb7d3 100644 --- a/main_test.go +++ b/main_test.go @@ -33,6 +33,8 @@ func TestReleaseProjectSkipPublish(t *testing.T) { } func TestConfigFileIsSetAndDontExist(t *testing.T) { + _, back := setup(t) + defer back() params := testParams() params.Config = "/this/wont/exist" assert.Error(t, releaseProject(params)) @@ -71,6 +73,8 @@ func TestConfigFileDoesntExist(t *testing.T) { } func TestReleaseNotesFileDontExist(t *testing.T) { + _, back := setup(t) + defer back() params := testParams() params.ReleaseNotes = "/this/also/wont/exist" assert.Error(t, releaseProject(params)) @@ -127,7 +131,6 @@ func TestInitProjectDefaultPipeFails(t *testing.T) { func testParams() releaseOptions { return releaseOptions{ - Debug: true, Parallelism: 4, Snapshot: true, Timeout: time.Minute, diff --git a/pkg/context/context.go b/pkg/context/context.go index ca6d7726c..40df0d32b 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -40,7 +40,6 @@ type Context struct { SkipSign bool SkipValidate bool RmDist bool - Debug bool PreRelease bool Parallelism int Semver Semver