From a0c58c74ace0aaa5729f8a7ce9514a3e56b662e8 Mon Sep 17 00:00:00 2001 From: Thulio Ferraz Assis <3149049+f0rmiga@users.noreply.github.com> Date: Wed, 12 Jan 2022 08:45:02 -0800 Subject: [PATCH] refactor: command hooks as interceptors (#104) --- cmd/aspect/build/BUILD.bazel | 2 - cmd/aspect/build/build.go | 24 +-------- cmd/aspect/root/BUILD.bazel | 2 +- cmd/aspect/root/root.go | 2 +- cmd/aspect/test/BUILD.bazel | 2 - cmd/aspect/test/test.go | 23 +-------- {cmd => pkg}/aspect/root/flags/BUILD.bazel | 4 +- {cmd => pkg}/aspect/root/flags/config.go | 0 pkg/plugin/system/BUILD.bazel | 1 + pkg/plugin/system/system.go | 60 +++++++++++++++------- 10 files changed, 49 insertions(+), 71 deletions(-) rename {cmd => pkg}/aspect/root/flags/BUILD.bazel (53%) rename {cmd => pkg}/aspect/root/flags/config.go (100%) diff --git a/cmd/aspect/build/BUILD.bazel b/cmd/aspect/build/BUILD.bazel index 753d4a5bc..8d7c3374c 100644 --- a/cmd/aspect/build/BUILD.bazel +++ b/cmd/aspect/build/BUILD.bazel @@ -6,9 +6,7 @@ go_library( importpath = "aspect.build/cli/cmd/aspect/build", visibility = ["//cmd/aspect/root:__pkg__"], deps = [ - "//cmd/aspect/root/flags", "//pkg/aspect/build", - "//pkg/aspecterrors", "//pkg/bazel", "//pkg/interceptors", "//pkg/ioutils", diff --git a/cmd/aspect/build/build.go b/cmd/aspect/build/build.go index de56c2c49..a59c957a9 100644 --- a/cmd/aspect/build/build.go +++ b/cmd/aspect/build/build.go @@ -8,14 +8,10 @@ package build import ( "context" - "errors" - "fmt" "github.com/spf13/cobra" - rootFlags "aspect.build/cli/cmd/aspect/root/flags" "aspect.build/cli/pkg/aspect/build" - "aspect.build/cli/pkg/aspecterrors" "aspect.build/cli/pkg/bazel" "aspect.build/cli/pkg/interceptors" "aspect.build/cli/pkg/ioutils" @@ -48,27 +44,9 @@ func NewBuildCmd( []interceptors.Interceptor{ interceptors.WorkspaceRootInterceptor(), pluginSystem.BESBackendInterceptor(), + pluginSystem.BuildHooksInterceptor(streams), }, func(ctx context.Context, cmd *cobra.Command, args []string) (exitErr error) { - isInteractiveMode, err := cmd.Root().PersistentFlags().GetBool(rootFlags.InteractiveFlagName) - if err != nil { - return err - } - - // TODO(f0rmiga): test this post-build hook. - defer func() { - errs := pluginSystem.ExecutePostBuild(isInteractiveMode).Errors() - if len(errs) > 0 { - for _, err := range errs { - fmt.Fprintf(streams.Stderr, "Error: failed to run build command: %v\n", err) - } - var err *aspecterrors.ExitError - if errors.As(exitErr, &err) { - err.ExitCode = 1 - } - } - }() - workspaceRoot := ctx.Value(interceptors.WorkspaceRootKey).(string) bzl.SetWorkspaceRoot(workspaceRoot) b := build.New(streams, bzl) diff --git a/cmd/aspect/root/BUILD.bazel b/cmd/aspect/root/BUILD.bazel index 2dea996e1..1e14375fd 100644 --- a/cmd/aspect/root/BUILD.bazel +++ b/cmd/aspect/root/BUILD.bazel @@ -13,11 +13,11 @@ go_library( "//cmd/aspect/clean", "//cmd/aspect/docs", "//cmd/aspect/info", - "//cmd/aspect/root/flags", "//cmd/aspect/run", "//cmd/aspect/test", "//cmd/aspect/version", "//docs/help/topics", + "//pkg/aspect/root/flags", "//pkg/ioutils", "//pkg/plugin/system", "@com_github_fatih_color//:color", diff --git a/cmd/aspect/root/root.go b/cmd/aspect/root/root.go index 97f9e7396..264d48412 100644 --- a/cmd/aspect/root/root.go +++ b/cmd/aspect/root/root.go @@ -19,11 +19,11 @@ import ( "aspect.build/cli/cmd/aspect/clean" "aspect.build/cli/cmd/aspect/docs" "aspect.build/cli/cmd/aspect/info" - "aspect.build/cli/cmd/aspect/root/flags" "aspect.build/cli/cmd/aspect/run" "aspect.build/cli/cmd/aspect/test" "aspect.build/cli/cmd/aspect/version" "aspect.build/cli/docs/help/topics" + "aspect.build/cli/pkg/aspect/root/flags" "aspect.build/cli/pkg/ioutils" "aspect.build/cli/pkg/plugin/system" ) diff --git a/cmd/aspect/test/BUILD.bazel b/cmd/aspect/test/BUILD.bazel index 5165522b2..cfc93e2a7 100644 --- a/cmd/aspect/test/BUILD.bazel +++ b/cmd/aspect/test/BUILD.bazel @@ -6,9 +6,7 @@ go_library( importpath = "aspect.build/cli/cmd/aspect/test", visibility = ["//visibility:public"], deps = [ - "//cmd/aspect/root/flags", "//pkg/aspect/test", - "//pkg/aspecterrors", "//pkg/bazel", "//pkg/interceptors", "//pkg/ioutils", diff --git a/cmd/aspect/test/test.go b/cmd/aspect/test/test.go index 37bacfe7f..be0e01505 100644 --- a/cmd/aspect/test/test.go +++ b/cmd/aspect/test/test.go @@ -8,14 +8,10 @@ package test import ( "context" - "errors" - "fmt" "github.com/spf13/cobra" - rootFlags "aspect.build/cli/cmd/aspect/root/flags" "aspect.build/cli/pkg/aspect/test" - "aspect.build/cli/pkg/aspecterrors" "aspect.build/cli/pkg/bazel" "aspect.build/cli/pkg/interceptors" "aspect.build/cli/pkg/ioutils" @@ -56,26 +52,9 @@ specify targets. []interceptors.Interceptor{ interceptors.WorkspaceRootInterceptor(), pluginSystem.BESBackendInterceptor(), + pluginSystem.TestHooksInterceptor(streams), }, func(ctx context.Context, cmd *cobra.Command, args []string) (exitErr error) { - isInteractiveMode, err := cmd.Root().PersistentFlags().GetBool(rootFlags.InteractiveFlagName) - if err != nil { - return err - } - - defer func() { - errs := pluginSystem.ExecutePostTest(isInteractiveMode).Errors() - if len(errs) > 0 { - for _, err := range errs { - fmt.Fprintf(streams.Stderr, "Error: failed to run test command: %v\n", err) - } - var err *aspecterrors.ExitError - if errors.As(exitErr, &err) { - err.ExitCode = 1 - } - } - }() - workspaceRoot := ctx.Value(interceptors.WorkspaceRootKey).(string) bzl.SetWorkspaceRoot(workspaceRoot) t := test.New(streams, bzl) diff --git a/cmd/aspect/root/flags/BUILD.bazel b/pkg/aspect/root/flags/BUILD.bazel similarity index 53% rename from cmd/aspect/root/flags/BUILD.bazel rename to pkg/aspect/root/flags/BUILD.bazel index f88d95c21..507aea0a9 100644 --- a/cmd/aspect/root/flags/BUILD.bazel +++ b/pkg/aspect/root/flags/BUILD.bazel @@ -3,6 +3,6 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( name = "flags", srcs = ["config.go"], - importpath = "aspect.build/cli/cmd/aspect/root/flags", - visibility = ["//visibility:public"], + importpath = "aspect.build/cli/pkg/aspect/root/flags", + visibility = ["//:__subpackages__"], ) diff --git a/cmd/aspect/root/flags/config.go b/pkg/aspect/root/flags/config.go similarity index 100% rename from cmd/aspect/root/flags/config.go rename to pkg/aspect/root/flags/config.go diff --git a/pkg/plugin/system/BUILD.bazel b/pkg/plugin/system/BUILD.bazel index 3454f91b2..fb085ad07 100644 --- a/pkg/plugin/system/BUILD.bazel +++ b/pkg/plugin/system/BUILD.bazel @@ -9,6 +9,7 @@ go_library( importpath = "aspect.build/cli/pkg/plugin/system", visibility = ["//visibility:public"], deps = [ + "//pkg/aspect/root/flags", "//pkg/aspecterrors", "//pkg/interceptors", "//pkg/ioutils", diff --git a/pkg/plugin/system/system.go b/pkg/plugin/system/system.go index ce56da456..417f7cdac 100644 --- a/pkg/plugin/system/system.go +++ b/pkg/plugin/system/system.go @@ -8,14 +8,17 @@ package system import ( "context" + "errors" "fmt" "os/exec" + "reflect" "time" hclog "github.com/hashicorp/go-hclog" goplugin "github.com/hashicorp/go-plugin" "github.com/spf13/cobra" + rootFlags "aspect.build/cli/pkg/aspect/root/flags" "aspect.build/cli/pkg/aspecterrors" "aspect.build/cli/pkg/interceptors" "aspect.build/cli/pkg/ioutils" @@ -30,8 +33,8 @@ type PluginSystem interface { Configure(streams ioutils.Streams) error TearDown() BESBackendInterceptor() interceptors.Interceptor - ExecutePostBuild(isInteractiveMode bool) *aspecterrors.ErrorList - ExecutePostTest(isInteractiveMode bool) *aspecterrors.ErrorList + BuildHooksInterceptor(streams ioutils.Streams) interceptors.Interceptor + TestHooksInterceptor(streams ioutils.Streams) interceptors.Interceptor } type pluginSystem struct { @@ -145,26 +148,47 @@ func (ps *pluginSystem) BESBackendInterceptor() interceptors.Interceptor { } } -// ExecutePostBuild executes all post-build hooks from all plugins. -func (ps *pluginSystem) ExecutePostBuild(isInteractiveMode bool) *aspecterrors.ErrorList { - errors := &aspecterrors.ErrorList{} - for node := ps.plugins.head; node != nil; node = node.next { - if err := node.plugin.PostBuildHook(isInteractiveMode, ps.promptRunner); err != nil { - errors.Insert(err) - } - } - return errors +// BuildHooksInterceptor returns an interceptor that runs the pre and post-build +// hooks from all plugins. +func (ps *pluginSystem) BuildHooksInterceptor(streams ioutils.Streams) interceptors.Interceptor { + return ps.commandHooksInterceptor("PostBuildHook", streams) +} + +// TestHooksInterceptor returns an interceptor that runs the pre and post-test +// hooks from all plugins. +func (ps *pluginSystem) TestHooksInterceptor(streams ioutils.Streams) interceptors.Interceptor { + return ps.commandHooksInterceptor("PostTestHook", streams) } -// ExecutePostTest executes all post-build hooks from all plugins. -func (ps *pluginSystem) ExecutePostTest(isInteractiveMode bool) *aspecterrors.ErrorList { - errors := &aspecterrors.ErrorList{} - for node := ps.plugins.head; node != nil; node = node.next { - if err := node.plugin.PostTestHook(isInteractiveMode, ps.promptRunner); err != nil { - errors.Insert(err) +func (ps *pluginSystem) commandHooksInterceptor(methodName string, streams ioutils.Streams) interceptors.Interceptor { + return func(ctx context.Context, cmd *cobra.Command, args []string, next interceptors.RunEContextFn) (exitErr error) { + isInteractiveMode, err := cmd.Root().PersistentFlags().GetBool(rootFlags.InteractiveFlagName) + if err != nil { + return fmt.Errorf("failed to run 'aspect %s' command: %w", cmd.Use, err) } + + // TODO(f0rmiga): test this hook. + defer func() { + hasErrors := false + for node := ps.plugins.head; node != nil; node = node.next { + params := []reflect.Value{ + reflect.ValueOf(isInteractiveMode), + reflect.ValueOf(ps.promptRunner), + } + if err := reflect.ValueOf(node.plugin).MethodByName(methodName).Call(params)[0].Interface(); err != nil { + fmt.Fprintf(streams.Stderr, "Error: failed to run 'aspect %s' command: %v\n", cmd.Use, err) + hasErrors = true + } + } + if hasErrors { + var err *aspecterrors.ExitError + if errors.As(exitErr, &err) { + err.ExitCode = 1 + } + } + }() + return next(ctx, cmd, args) } - return errors } // ClientFactory hides the call to goplugin.NewClient.