[ORCA-4531] Improve pre workflow hook error handling #228
Conversation
msarvar
force-pushed
the
improve-pre-workflow-hook-error-handling
branch
from
7fd0a3c
to
a105dc3
Compare
msarvar
changed the title
server/events/command_runner.go
Outdated
| @@ -179,7 +179,11 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(logger logging.SimpleLogging, | |||
| err = c.PreWorkflowHooksCommandRunner.RunPreHooks(ctx) | |||
|
|
|||
| if err != nil { | |||
| ctx.Log.Errorf("Error running pre-workflow hooks %s. Proceeding with %s command.", err, command.Plan) | |||
| ctx.Log.Errorf("Error running pre-workflow hooks %s. Aborting %s command.", err, command.Plan.String()) | |||
There was a problem hiding this comment.
can you use my new logger here pls.
There was a problem hiding this comment.
Also no formatting allowed in logs anymore.
There was a problem hiding this comment.
Are you talking about logging.Logger interface and do I need to thread it into command runner?
server/events/command_runner.go
Outdated
| if err := c.VCSClient.CreateComment(ctx.Pull.BaseRepo, ctx.Pull.Num, fmt.Sprintf("Encountered an error during pre-workflow-hook execution: %s", err), command.Plan.String()); err != nil { | ||
| ctx.Log.Errorf("unable to comment: %s", err) | ||
| } |
There was a problem hiding this comment.
I'd like to move away from this paradigm of commenting logging errors on the PR since its just noise that the customer won't understand and just pollutes our codebase all over the place.
The ideal UX in my mind is to mark the status/checks as fail and have some details as to why they failed when you click into it. For now i think just failing the statuses is enough since customers will need our help anyways at that point.
| } | ||
|
|
||
| func (r *PreWorkflowHookRunner) RunPreHooks(ctx *command.Context) error { | ||
| scope := ctx.Scope.SubScope("pre-workflow-hook") |
There was a problem hiding this comment.
i think we've been using underscores for metric names instead of dashes.
msarvar
force-pushed
the
improve-pre-workflow-hook-error-handling
branch
from
9601e54
to
09714de
Compare
server/events/command/context.go
Outdated
| User models.User | ||
| Log logging.SimpleLogging | ||
| User models.User | ||
| Log logging.SimpleLogging |
There was a problem hiding this comment.
can you remove this? Why have both? We're gonna need to do this for the whole codebase at some point anyways, might as well do it per file as we come across it.
server/events/command_runner.go
Outdated
| ctx.Log.Errorf("Error running pre-workflow hooks %s. Proceeding with %s command.", err, command.Plan) | ||
| if err := c.PreWorkflowHooksCommandRunner.RunPreHooks(context.TODO(), ctx); err != nil { | ||
| c.Logger.ErrorContext(context.TODO(), "Error running pre-workflow hooks", map[string]interface{}{ | ||
| "err": err, |
There was a problem hiding this comment.
can you add repository and pull num here? similar to here:
https://github.com/lyft/atlantis/blob/release-v0.17.3-lyft.1/server/events/vcs/instrumented_client.go#L346-L348
Eventually this can be added to ctx and ripped out from there automatically but for now we need this.
There was a problem hiding this comment.
yeah, my thinking was that it will be covered by the context, but you're right in the meantime we need a workaround.
| return err | ||
| } | ||
|
|
||
| r.Logger.InfoContext(ctx, "pre-workflow-hook success", logHelpers.PullRequest(cmdCtx.Pull)) |
There was a problem hiding this comment.
not a fan of helpers, handlers, util, common etc. it's not descriptive and becomes a dumping ground.
here, it could make sense to just use fields as the package. so it would be fields.PullRequest(..)
Again this should hopefully go away soon. Can we add a //TODO here to remove that
There was a problem hiding this comment.
done, also added todo to the package
server/logging/logger.go
Outdated
| // writes them. Used for testing. | ||
| func NewNoopCtxLogger(t *testing.T) Logger { | ||
| return &logger{ | ||
| LoggerFacade: logur.NoopLogger{}, |
There was a problem hiding this comment.
What does this NoopLogger do btw? The noop logger we have is nice because it logs all the output to t so you can at least debug failing tests.
There was a problem hiding this comment.
It literally did nothing, changed to mimic our NoopLogger
use our NoopLogger instead of logur.NoopLogger
msarvar
force-pushed
the
improve-pre-workflow-hook-error-handling
branch
from
902fc6f
to
d522a3a
Compare
Updating our pre workflow hooks logic to fail the workflow execution when hook return an error. This will provide better user experience and will allow us to debug issue related to pre-workflow-hook failures.
Additionally added instrumentation to pre workflow hooks to allow us to create alarms when pre workflow hooks fail.