Skip to content
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

Merged
merged 10 commits into from
Apr 3, 2023

Conversation

fabriziosestito
Copy link
Member

@fabriziosestito fabriziosestito commented Mar 29, 2023

Refactor web error handling:

  • Make controllers agnostic of domain error
  • Make fallback controller aware of domain error
  • Fix error handling in certain contexts, improve logging
  • Add changeset rendering in ErrorView
  • use a tagged tuple to return validation errors

@fabriziosestito fabriziosestito force-pushed the refactor-contexts-controllers-errors branch from 8549de3 to 973a7af Compare March 29, 2023 11:25
@fabriziosestito fabriziosestito force-pushed the refactor-contexts-controllers-errors branch from 13d220a to c447bc1 Compare March 30, 2023 14:27
@fabriziosestito fabriziosestito changed the title Refactor cluster controller and context error handling Refactor controllers and contexts error handling Mar 30, 2023
@fabriziosestito fabriziosestito marked this pull request as ready for review March 30, 2023 14:29
Copy link
Contributor

@arbulu89 arbulu89 left a 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(
Copy link
Contributor

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?

Copy link
Member Author

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
Copy link
Contributor

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

Copy link
Member Author

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

@fabriziosestito fabriziosestito force-pushed the refactor-contexts-controllers-errors branch from c447bc1 to 8b7647d Compare March 30, 2023 15:24
@fabriziosestito
Copy link
Member Author

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

@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.

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, thanks!

@fabriziosestito fabriziosestito merged commit efb0d71 into main Apr 3, 2023
@fabriziosestito fabriziosestito deleted the refactor-contexts-controllers-errors branch April 3, 2023 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants