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

Make Software Updates service handle no SUMA settings saved #2457

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

jamie-suse
Copy link
Contributor

Description

This changes allows the Software Updates service to return an error with a response status of 422 and provides the message "SUSE Manager settings not configured.".

How was this tested?

Updated existing unit tests to validate new behaviour.

@jamie-suse jamie-suse added enhancement New feature or request elixir Pull requests that update Elixir code labels Mar 21, 2024
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.

LGTM.

@jamie-suse jamie-suse self-assigned this Mar 21, 2024
@jamie-suse jamie-suse force-pushed the handle-no-saved-suma-settings branch from b45eecd to d0d8b06 Compare March 21, 2024 16:37
Copy link
Contributor

@dottorblaster dottorblaster left a 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 👍

Comment on lines 126 to 127
else
{:error, reason} -> {:error, :unable_to_get_software_updates, reason}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
else
{:error, reason} -> {:error, :unable_to_get_software_updates, reason}

Comment on lines 20 to 29
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

Copy link
Contributor

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?

@nelsonkopliku
Copy link
Member

nelsonkopliku commented Mar 22, 2024

hey @dottorblaster, Jamie and I paired a bit on this, so let me share some rationale:
there was an initial requirement of differentiating in the UI SUMA not available vs host not found on SUMA, so we thought of contextualizing the failure with :unable_to_get_software_updates + reason in order to support differentiating the reason why the operation of getting software updates for a host failed.

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 :settings_not_configured as a possible failure, but since :settings_not_configured is already handled somewhere else in the fallback controller (operations on the credentials resource), without some extra contextualization we'd get on the get software updates for a host endpoint a 404 The requested resource cannot be found. both in case settings were not configured and if the requested host id is not in trento.
In all other cases a 422 with a tiny more specific error message is returned.
And I'd be fine with it.

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 😉

Copy link
Contributor

@dottorblaster dottorblaster left a 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!

@jamie-suse jamie-suse merged commit 45d332e into main Mar 22, 2024
24 checks passed
@jamie-suse jamie-suse deleted the handle-no-saved-suma-settings branch March 22, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

3 participants