-
Notifications
You must be signed in to change notification settings - Fork 15
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
Refactor controllers and contexts error handling #1290
Conversation
8549de3
to
973a7af
Compare
13d220a
to
c447bc1
Compare
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.
Cool!
As you have added/updated error logging, did you check if we are capturing them in the tests? Just to have a clean tests output, and not filled with "expected" error messages
@@ -71,7 +82,9 @@ defmodule Trento.Clusters do | |||
:ok | |||
|
|||
{:error, reason} -> | |||
Logger.error("Failed to request checks execution for cluster #{cluster_id}: #{reason}") | |||
Logger.error( |
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 something you should do in this PR, but we should reduce the errors in this each
now and return error if any of them fails.
Or this return the error if fails?
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.
this is used by the job scheduler in a best-effort manner, this is why we just use the each loop and we return nothing.
If something went wrong while scheduling the execution we log it
end | ||
|
||
def call(conn, {:error, {:unauthorized, detail}}) do | ||
def call(conn, {:error, reason}) | ||
when reason in [:host_not_registered, :cluster_not_registered, :sap_system_not_registered] do |
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 hope this list doesn't convert on a root for bugs hehe
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 double-checked photofinish and the agent and it should be ok
c447bc1
to
8b7647d
Compare
@arbulu89 I will open a new PR because I believe there is a way to capture logs globally, so we do not have to do this to specific tests. |
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.
Nice job, thanks!
Refactor web error handling: