Error system and error handling of AssignmentService by Thorben-D · Pull Request #7 · ATIX-AG/foreman_ansible_director

Thorben-D · 2026-02-10T11:18:42Z

Copied from "feat(errors): add custom error system":

This system allows developers to wrap code in a function which
takes an error scope.
This scope can contain one or more application errors, each of which
represent a step that can go wrong when the code is executed.

When one of the errors expected in the scope is raised, it is resolved
to a customized error message, which contains:

a HTTP response code
a title
a description
a link to documentation explaining the error

This error is then raised and bubbled up to the controller, where it is
returned to the user.
Should an error be raised that is not expected in the scope, an UnexpectedError
exception will be raised, which tells the developer that a known error was
raised in a scope it was not expected to occur in.
Should a non-ApplicationError exception be raised, an UnknownError will be
raised, telling the user that an actual bug is present.

Furthermore, this PR makes use of that system for the AssignmentService.

Requires: #6
Requires: #1

This system allows developers to wrap code in a function which takes an error scope. This scope can contain one or more application errors, each of which represent a step that can go wrong when the code is executed. When one of the errors expected in the scope is raised, it is resolved to a customized error message, which contains: - a HTTP response code - a title - a description - a link to documentation explaining the error This error is then raised and bubbled up to the controller, where it is returned to the user. Should an error be raised that is not expected in the scope, an UnexpectedError exception will be raised, which tells the developer that a known error was raised in a scope it was not expected to occur in. Should a non-ApplicationError exception be raised, an UnknownError will be raised, telling the user that an actual bug is present.

- finder: - (ADR-003-005-004-001) invalid target type Raised when the user supplied an unsupported target type - find_target: - (ADR-003-005-005-001) target not found Raised when the user supplied a target id which does not exist Introduces error codes: - ADR-003-005-004-001 - ADR-003-005-005-001

…vice This new base-class allows definition of service-global methods and imports. Includes Ansible director error helpers.

Usage of find_target enables error-handling and deduplicates code.

sbernhard · 2026-02-12T11:38:31Z

@@ -0,0 +1,65 @@
+# frozen_string_literal: true


I don't think we should have "our own" erorr code system.

Please have a look at: https://github.com/theforeman/foreman/blob/develop/lib/foreman/exception.rb#L8

if we want to have something better than this, we should take the time to get this into foreman core so that other plugins (and core) can re-use this.

I disagree for multiple reasons:

The error code calulated by the function in core serves, at best, as a lookup for the controller. And even for that, a lookup table is required.

Said table can only be generated for core, which is why Foreman::Exception is not used in plugins.

Any upstream work is not really in the scope of this PR. Using our system in an existing plugin would require extensive work.

Lastly, this is pretty much just an abstraction over the common begin-rescue strategy used in plugins.

Why not just throw an exception without these error codes?

General benefits of error codes aside, raising an exception in the business logic would require bloating of the BL with a message, a response code and a title.
This already fixes that.

sbernhard · 2026-02-26T13:47:29Z

+          end
+          # rubocop:enable Style/GuardClause
+        rescue StandardError
+          raise ::ForemanAnsibleDirector::Errors::UnknownError


instead of hiding the real issue, what about just let it unhandled and pass it through?

sbernhard · 2026-02-26T13:48:33Z

+        end
+      end
+
+      class ErrorScopeRescue


this is currently not used. what is the purpose of this and how should it work? I think it would be better to handle the exception at the code section where they really happen

sbernhard · 2026-02-26T13:49:40Z

@@ -0,0 +1,18 @@
+003:


this file needs to be auto-generated from the error objects

sbernhard · 2026-02-26T13:52:44Z

-        # TODO: Null check target
        finder = finder(type: target_type)
-        finder.find_by(id: target_id)
+        error_scope(['ADR-003-005-005-001']).try do


this duplicates the error codes like this https://github.com/ATIX-AG/foreman_ansible_director/pull/7/changes#diff-3fd51f00ed42919881336d2ff09ded12a6ced416213ec0dcb811917d5bfb7c44R61

I know, its about tell error_scope which error may occur. unknown error should "flow through"

sbernhard · 2026-02-26T13:57:52Z

+    end
+
+    ERROR_OBJECTS = HashWithIndifferentAccess.new({
+      "ADR-003-005-004-001": {


please use a easier error code scheme like "ADR-001-001" instead of 4 columns

maximiliankolb · 2026-03-02T14:40:01Z

As a user, I would appreciate a good error message.

Example: If I create an Ansible Execution Environment via WebUI & do not provide a string in "Base image URL", the UI shows Creation of Ansible execution environment "example" failed with error code "500".

Instead, it would be great to read "Error: Base image URL cannot be empty: Please provide a base-image URL."

maximiliankolb · 2026-03-03T10:55:28Z

@sbernhard & @Thorben-D Another example:

Importing of 1 Ansible content units failed with error code "500". <- error if you enter a non-existing Ansible collection. As a user, I want the error message to read Ansible API "galaxy.ansible.com" does not provide Ansible Collection "X.Y". Error code 500 is not helpful at all.

maximiliankolb · 2026-03-13T07:31:34Z

$ curl --user "admin:changeme" https://foreman.example.com/api/v2/ansible_director/lifecycle_environments/9
{"status":500,"error":"Internal Server Error"}

This error happened when I tried to query the API but mistakenly took the lifecycle environment path ID instead of the lifecycle environment ID. As a user, I though that my Foreman+Katello instance ran into an error aka. there is an issue in the code.

It would be so more nicer if the JSON returned the same status but a useful error message, for example, "Lifecycle environment ID not found.".

cc @sbernhard & @Thorben-D

maximiliankolb · 2026-03-16T13:19:38Z

I found a great error message example in Ansible Director:

$ cat data.json
{
  "execution_environment": {
    "name": "my-aee",
    "ansible_version": "1.2.3"
  }
}
$ curl --data @data.json --header "Accept:application/json" --header "Content-Type:application/json" --request PATCH --user "admin:changeme" https://foreman.example.com/api/v2/ansible_director/execution_environments/1
{
  "error": {"message":"Validation failed: Base image URL Execution Environment base image URL cannot be blank."}
}

Possible minor improvement: Add the name of the key "base_image_url" to the error message. But it's already extremely helpful when debugging issue to understand what failed, why, and how to fix it.

maximiliankolb · 2026-04-21T14:00:52Z

@sbernhard & @jtruestedt

Please prioritze having error messages in Ansible Director.

{
  "error": {
    "message": "Task bc31a2ec-e778-4ec6-95d7-a2bb56d546bd: NoMethodError: undefined method `each' for nil:NilClass"
  }
}

Is the return value of the API on nightly if you try to import an Ansible collection from Galaxy that is already present. As a user, this is less than ideal.

sbernhard · 2026-04-21T16:00:32Z

Instead of implementing our own error code system, please use named exceptions as this is used in foreman, katello and other plugins. Like: https://github.com/Katello/katello/blob/master/app/lib/katello/errors.rb

Thorben-D force-pushed the error_system branch 2 times, most recently from aac9690 to 50f3f7a Compare February 10, 2026 11:54

Thorben-D mentioned this pull request Feb 10, 2026

Assignment logic refactor and testing #6

Merged

Thorben-D added 4 commits February 10, 2026 15:02

refactor(services): make all services inherit from AnsibleDirectorSer…

4457bef

…vice This new base-class allows definition of service-global methods and imports. Includes Ansible director error helpers.

refactor(services/assignment): use find_target in create_bulk_overrides

6dfa054

Usage of find_target enables error-handling and deduplicates code.

Thorben-D force-pushed the error_system branch from 50f3f7a to 6dfa054 Compare February 10, 2026 14:02

Thorben-D mentioned this pull request Feb 12, 2026

Search / autocomplete support for API queries and variables UI #15

Merged

sbernhard reviewed Feb 12, 2026

View reviewed changes

sbernhard reviewed Feb 26, 2026

View reviewed changes

Comment thread error_codes.yaml

@@ -0,0 +1,18 @@

003:

Copy link
Copy Markdown

Member

sbernhard Feb 26, 2026

Choose a reason for hiding this comment

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

this file needs to be auto-generated from the error objects

sbernhard reviewed Feb 26, 2026

View reviewed changes

Thorben-D marked this pull request as draft February 27, 2026 11:40

Conversation

Thorben-D commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbernhard Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maximiliankolb commented Mar 2, 2026

Uh oh!

maximiliankolb commented Mar 3, 2026

Uh oh!

maximiliankolb commented Mar 13, 2026

Uh oh!

maximiliankolb commented Mar 16, 2026

Uh oh!

maximiliankolb commented Apr 21, 2026

Uh oh!

sbernhard commented Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbernhard Feb 26, 2026 •

edited

Loading