-
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
Make Software Updates
service handle no SUMA settings saved
#2457
Conversation
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.
LGTM.
b45eecd
to
d0d8b06
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.
Great job! Just a couple things then we can merge 👍
lib/trento/software_updates.ex
Outdated
else | ||
{:error, reason} -> {:error, :unable_to_get_software_updates, reason} |
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.
else | |
{:error, reason} -> {:error, :unable_to_get_software_updates, reason} |
def call(conn, {:error, :unable_to_get_software_updates, :not_found}), | ||
do: call(conn, {:error, :not_found}) | ||
|
||
def call(conn, {:error, :unable_to_get_software_updates, reason}) do | ||
conn | ||
|> put_status(:unprocessable_entity) | ||
|> put_view(ErrorView) | ||
|> render(:"422", reason: get_failure_message(reason)) | ||
end | ||
|
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 actually like single clauses to be more explicit. A fallback controller shouldn't manage this kind of errors in an implicit way, Could you just declare one more clause instead of having this refactoring?
hey @dottorblaster, Jamie and I paired a bit on this, so let me share some rationale: Since it seems (@jamie-suse could you confirm, please?) that this differentiation is not strictly needed now (and it can be done in many ways) I believe we could defer any further fine grained error handling for now. That said, we surely can just add In any case I guess we have room to improve error handling, and this relates also to the discussion you raised some days ago that ended up in the possibility to have contextual fallback controller. But let's of course defer that 😄 Hope the context helps 😉 |
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.
@nelsonkopliku all clear, thank you! Of course the status quo always has room for improvement, let's keep this on the radar, thank you so much!
Description
This changes allows the
Software Updates
service to return an error with a response status of422
and provides the message "SUSE Manager settings not configured.".How was this tested?
Updated existing unit tests to validate new behaviour.