-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
feat: Refine the Atlantis Info Message Logging #5034
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: X-Guardian <[email protected]>
Signed-off-by: X-Guardian <[email protected]>
Signed-off-by: X-Guardian <[email protected]>
Signed-off-by: X-Guardian <[email protected]>
Signed-off-by: X-Guardian <[email protected]>
Signed-off-by: X-Guardian <[email protected]>
Signed-off-by: Simon Heather <[email protected]>
if err != nil { | ||
ctx.Log.Err("Error running post-workflow hooks %s.", err) | ||
} | ||
a.PostWorkflowHooksCommandRunner.RunPostHooks(ctx, cc[i]) // nolint: errcheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why I follow why these logs are being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They have been moved into events/post_workflow_hooks_command_runner.go
rather than being duplicated here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Always a bit apprehensive when an err
is being swallowed though; if we expect the body of RunPostHooks to "handle" the error (i.e. logging it), and we've now change all the call sites of this function to ignore the error, maybe it shouldn't return an error at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests use the return error, so we should leave it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lukemassa, any chance we can get this PR over the line? I'm not really bothered about de-duping these log messages, so if you feel strongly about this, I'll just revert my changes. I want to get the other enhanced logging through.
@@ -344,7 +344,7 @@ func TestDefaultClient_RunCommandAsync_ExitOne(t *testing.T) { | |||
_, outCh := client.RunCommandAsync(ctx, tmp, []string{"dying", "&&", "exit", "1"}, map[string]string{}, nil, "workspace") | |||
|
|||
out, err := waitCh(outCh) | |||
ErrEquals(t, fmt.Sprintf(`running 'sh -c "echo dying && exit 1"' in '%s': exit status 1`, tmp), err) | |||
ErrEquals(t, fmt.Sprintf(`running 'sh -c echo dying && exit 1' in '%s': exit status 1`, tmp), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the quotes here makes this harder to read. sh -c echo dying && exit 1
means a different thing than sh -c "echo dying && exit 1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the JSON logs, the msg
property currently looks like this:
"msg":"successfully ran 'sh -c \"/root/.atlantis/bin/terraform1.9.8 init -input=false -upgrade\"' in '/root/.atlantis/repos/sheather/test/31/default/live/aws/stack11'"
which with the escaped double quotes is very confusing to read, so I removed them to give this:
"msg":"successfully ran 'sh -c /root/.atlantis/bin/terraform1.9.8 init -input=false -upgrade' in '/root/.atlantis/repos/sheather/test/31/default/live/aws/stack11'"
I can replace it with single quotes, or backticks instead:
Single Quotes
"msg":"successfully ran 'sh -c '/root/.atlantis/bin/terraform1.9.8 init -input=false -upgrade'' in '/root/.atlantis/repos/sheather/test/31/default/live/aws/stack11'"
backticks
"msg":"successfully ran 'sh -c `/root/.atlantis/bin/terraform1.9.8 init -input=false -upgrade`' in '/root/.atlantis/repos/sheather/test/31/default/live/aws/stack11'"
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, that's tough.
My personal view is that if you're logging to JSON you can expect \"
s, and that it's still useful to be able to recover what command was actually ran.
But I could definitely see it going either way, and am fine with any of the above solutions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added single quotes.
Signed-off-by: X-Guardian <[email protected]>
what
Refine the Info logging messages that Atlantis produces in the following areas:
why
Improve the logging to help resolve future issues.
tests
Tested locally
Sample Logs
Current
New