checks for gitea PRs based on AMQP messages #272

d3flex · 2025-11-10T08:55:21Z

Add a handler on dedicate queue which will schedule openQA jobs on gitea review requests, if qam-openqa-review is part of the reviewers.

This is more or less upon the amqp-listen-gitea https://github.com/os-autoinst/os-autoinst-scripts/pull/468/files but without boilerplate of openqa, as the qem-bot already implements interface to openQA.
Using an additional parameter we can invoke the listener with a particular queue.

./bot-ng.py --debug --configs ../metadata -t 1234 --dry amqp --queue gitea

Without --queue both queues are created.
It would be nice to have an accordingly named queue but I didnt implement this. each queue receives automatically a generated name.

Now IIUC handle_incident retrieves inc settings and then tries to schedule job for each. So I couldnt use it directly. in my understanding, the review_request must trigger scheduler for particular set of settings. I am not sure if handle_inc_review_request handles this properly. it assumes that still the settings will be found in the dashboard and therefore it tries to fetch them using the same request.

I test this manual and following with some basic tests. I run the amqp command again as I write this, but I have no results yet. listening only to suse.src.pull_request_review_request.review_requested. this might be a little strict. I think the rabbitmq returns something which we can handle if we want to check that it does something underneath.

issue: https://progress.opensuse.org/issues/189942

d3flex · 2025-11-10T10:00:34Z

@foursixnine fyi

foursixnine · 2025-11-10T10:22:52Z

openqabot/amqp.py

+        try:
+            log.info(f"Scheduling jobs for PR {pr_number}")
+            error_count = 0
+            for s in settings_data:
+                params = {
+                    "DISTRI": s.distri,
+                    "VERSION": s.version,
+                    "FLAVOR": s.flavor,
+                    "ARCH": s.arch,
+                    "BUILD": s.build,
+                }
+                log.info(
+                    "Scheduling job for %s v%s build %s@%s of flavor %s", s.distri, s.version, s.build, s.arch, s.flavor
+                )
+                if self.dry:
+                    log.info("Dry run - would schedule: %s", params)
+                    continue
+                try:
+                    self.client.post_job(params)
+                except Exception as e:
+                    log.error("Failed to schedule job with params %s: %s", params, e)
+                    error_count += 1


In this case, we'd only would trigger openQA jobs if the build for the given PR is finished and perhaps if qam-openqa-review is within the requested reviewers.

https://github.com/os-autoinst/os-autoinst-scripts/pull/470/files#diff-bede70cd496a0e28689b3d1e2a08f5b3f0145580e90e235d1b6e78f08fe23657R462

Line 462 of amqp-autogits_workflow_pr_bot-pull_request_review_approved-20251006-154156.json contains an example of a successful build.

to make sure, the openqa tests should be triggered when

"review": { "type": "pull_request_review_approved", "content": "Build successful" }

is the routing key of this message suse.src.pull_request_review_request.review_requested or is something like suse.src.pull_request_review_request.review_approved?

I can't recall tbh

Martchus

In this form this PR seems rather incomplete. It only seems to do something for a new PR if incident settings are already present on the dashboard. So only if the periodically running pipeline has already done that it would help speed things up. However, then we probably also don't need it anymore.

To be actually useful this approach needed to start with what the "first" pipeline step (gitea-sync) does and only then do the next steps. (For all the steps you can checkout the commands for local testing which basically describe all the steps from creating an incident on the dashboard to the final approval.)

However, you of course had to start somewhere. I guess I would have started from the start creating an incident on the dashboard first but your changes can be extended. The only thing that is really wrong is the duplication of making the scheduling parameters.

By the way, goal 1 of https://progress.opensuse.org/issues/189942 is phrased very openly. So in theory also an approach that doesn't use the dashboard and existing code for computing parameters at all would be acceptable. However, I think if you involve the dashboard (which you did) then the parameters should also be computed using the existing logic. (Then this will basically be another (more event driven) way of doing what we already do with the full dashboard.)

Martchus · 2025-11-10T12:58:37Z

openqabot/amqp.py

        body: bytes,
    ) -> None:
+        log.info("%s - Received openQA job message", method.routing_key)
+        log.debug(" %s - %s", method.routing_key, body.decode())


Suggested change

log.debug(" %s - %s", method.routing_key, body.decode())

log.debug("%s - %s", method.routing_key, body.decode())

And we probably don't want method.routing_key again as it is already covered by the previous log message.

The same applies to the other occurrence.

space was intentional.
about the method.routing_key, I want it to be visible in the logs. if the amqp do not run with --queue it will be harder to spot where are the message coming from. there a logging formatter or something, but I didnt want to go that far for this ticket. it is supposed to show what I have in mind for the task, but not cover everything in the given timebox period.

Martchus · 2025-11-10T13:09:52Z

openqabot/amqp.py

+        log.info("%s - Received openQA job message", method.routing_key)
+        log.debug(" %s - %s", method.routing_key, body.decode())
        message = json.loads(body)
+        if self.args.dry:


It would make more sense to handle self.args.dry in a deeper level (maybe self.handle_incident) to we still run as much code as possible with --dry.

Martchus · 2025-11-10T13:10:47Z

openqabot/amqp.py

+        properties: pika.spec.BasicProperties,  # noqa: ARG002 Unused method argument
+        body: bytes,
+    ) -> None:
+        log.info("%s - Received gitea review request message", method.routing_key)


Suggested change

log.info("%s - Received gitea review request message", method.routing_key)

log.info("%s - Received Gitea review request message", method.routing_key)

Martchus · 2025-11-10T13:11:47Z

openqabot/amqp.py

+        log.info("%s - Received gitea review request message", method.routing_key)
+        log.debug(" %s - %s", method.routing_key, body.decode())
+        message = json.loads(body)
+        if self.args.dry:


Again, this could and should probably be handled at a deeper level.

Martchus · 2025-11-10T13:15:00Z

openqabot/amqp.py

+        if not pr_number:
+            log.error("Could not extract PR number from AMQP message")
+            return


I think int will throw a ValueError but never 0 (or another falsy value) if an argument is given. So this error handling is dead code.

Martchus · 2025-11-10T13:25:42Z

tests/fixtures/qembot_mocks.py

+
+
+@pytest.fixture
+def qem_mock_get_incident_settings() -> Any:


Suggested change

def qem_mock_get_incident_settings() -> Any:

def mock_get_incident_settings() -> Any:

or

Suggested change

def qem_mock_get_incident_settings() -> Any:

def mock_qem_get_incident_settings() -> Any:

The same applies to other functions. I'd still always start with mock_ for functions which do mocking and keep the location/name of the function being mocked together.

Martchus · 2025-11-10T13:27:29Z

tests/fixtures/qembot_mocks.py

+    """Fixture providing a mock for get_incident_settings_data function.
+    to make it function in multiple other test, as `get_incident_settings_data` you
+    and making it reusable across all tests append the patch with teh other places.


Please fix the spelling mistakes. If you don't want to, simply remove this whole documentation. We don't need documentation for this kind of function. Its name and location already tell what it is for so this whole paragraph doesn't add much value anyway.

Martchus · 2025-11-10T13:32:47Z

tests/fixtures/qembot_mocks.py

+        reviewer: The username of the requested reviewer (default: "qam-openqa-review")
+
+    Returns:
+        A json-encoded bytes object of gitea review request message


Suggested change

A json-encoded bytes object of gitea review request message

A JSON-encoded bytes object of a Gitea review request message

Martchus · 2025-11-10T13:36:33Z

tests/fixtures/qembot_mocks.py

+@pytest.fixture
+def gitea_mock_review_request_body() -> Any:
+    """Mock the review request body for Gitea responses.
+    Here is the minimum requirement for amqp event handling...


There's something missing here. However, I'm wondering whether we really need this lengthy documentation with even examples in the first place? It really doesn't add anything you can't just infer from looking a the names of the function and its arguments.

So I suggest: Keep it short and without typos or avoid these kinds of documentations.

By the way, a short version would be:

"Returns the review request body for the Gitea PR with the specified number and reviewer as JSON-encoded bytes object."

Martchus · 2025-11-10T13:39:58Z

tests/test_amqp.py

+# Check fixtures in tests/fixtures/qembot_mocks.py
+# imported from conftest.py


Where is this actually imported? If it isn't imported anywhere after all, then this comment should probably be removed.

is the term import the problem here? I just want to let you know where to find the mocks if you come in the the test for first time.
But they are imported in the conftest.
https://docs.pytest.org/en/7.1.x/reference/fixtures.html#conftest-py-sharing-fixtures-across-multiple-files

To me it wasn't even clear that these two lines from a whole sentence. What about using correct punctuation once in a while?

And it is also not clear that this is telling someone to check something. Normally comments describe what the code does in case that's not obvious. So rather say:

Fixtures for this test are imported via conftesyt.py and can be found in tests/fixtures/qembot_mocks.py.

d3flex · 2025-11-10T21:44:05Z

I push some changes. I will review/answer the comments tomorrow

Extend the amqp module with a separate queue for the gitea PRs. Following the existing design which provides the logic and the flexibility to orchestrate the event handling according their purpose. The existing queue is listening for suse.openqa events and nothing has changed in this flow, except some small tweaks, which most significant, to prevent normal execution when `dry` is enabled. All the events from `suse.src.pull_request_review_request.review_requested` should activate `handle_inc_review_request` which its rensposibility is to schedule jobs. This is true if the data contains `qam-openqa-review` in the reviewers. Here there are a few option, depending on the way the updates are being updated. - `do_incident_schedule` will check the dashboard and schedule everything. - `IncrementApprover._schedule_openqa_jobs` but it is an instance method - Likely the best option is to call `post_job` from openQAInterface The command line is append with `--queue` in order to be able to run the amqp command against a particular queue if this is wanted. However by default it initializes both defined queues. issue: https://progress.opensuse.org/issues/189942 Signed-off-by: Ioannis Bonatakis <[email protected]>

- Create fixtures for `get_incident_settings_data` and `post_job` - Setup provision of the mocks and fixtures to all the tests And tests cover: - a smoke test for dry_run - the positive scenario - and a negative scenarion where the `qam-openqa-review` is not in the reviewer issue: https://progress.opensuse.org/issues/189942 Signed-off-by: Ioannis Bonatakis <[email protected]>

codecov · 2025-11-12T12:22:23Z

Codecov Report

❌ Patch coverage is 69.44444% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.91%. Comparing base (c55b1fb) to head (f4a14db).
⚠️ Report is 48 commits behind head on master.

Files with missing lines	Patch %	Lines
openqabot/amqp.py	69.01%	22 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #272      +/-   ##
==========================================
- Coverage   75.12%   74.91%   -0.22%     
==========================================
  Files          32       32              
  Lines        2730     2794      +64     
==========================================
+ Hits         2051     2093      +42     
- Misses        679      701      +22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Depending on the service the scope is changing. For rabbit.suse.de is always `suse` and for rabbit.opensuse.org is always `opensuse`. So this commit allows to catch all the events despite which AMQP_URL is used. issue: https://progress.opensuse.org/issues/189942

To avoid depending on dashboard to get incident data, the handle_inc_review_request was refactored to make use of the `Incident` Class. The instance should compute the required variables to make a direct job_post and schedule job groups. Some variables are still hardcoded. And some bits might still missing, but the implementation attempts to reuse existing functionality. Tests have been adjusted. But there are unused mocks. Left intentionally in a place which tests can make use of them in the future, as they mock common function(s). issue: https://progress.opensuse.org/issues/189942 Signed-off-by: Ioannis Bonatakis <[email protected]>

foursixnine · 2025-11-13T15:55:58Z

openqabot/amqp.py

            return
+        routing_keys = {
+            "openqa": ("*.openqa.#", self.on_job_message),
+            "gitea": ("*.pull_request_review_request.#", self.on_review_request),


Keep in mind that the event you should be looking for, is the PR approval by the bot: https://src.opensuse.org/products/PackageHub/pulls/213#issuecomment-51757 (at least for openSUSE, Looks like for SLE is the same story) because it tells you that the build has finished, and is ok to trigger tests, otherwise the build might not be finished and the tests will fail.

I think the requirement is satisfied here https://github.com/openSUSE/qem-bot/pull/272/files#diff-78f57cc644e149681a7b2cc5aab3776d91e8ee1ccca16f29016ab983a5d46db5R115. specifically,

def _has_build_succeeded() -> bool: review_tag = message.get("review", "") if not review_tag or review_tag == "null": return False is_type_approved = review_tag.get("type") == "pull_request_review_approved" is_build_success = review_tag.get("content") == "Build successful" return is_type_approved and is_build_success

foursixnine · 2025-11-14T10:04:44Z

openqabot/amqp.py

+        log.debug(" %s - %s", method.routing_key, body.decode())
+        message = json.loads(body)
+
+        def _has_build_succeeded() -> bool:


I would also check that the user is autogits_obs_staging_bot

ok. But why? at the point where the _has_build_succeeded, why do I need the user? he has done whatever he has to do, no?

d3flex requested review from Martchus, baierjan, okurz and perlpunk November 10, 2025 09:37

d3flex force-pushed the feat/gitea_amqp_handler branch from 11ecfd5 to 63cbbc2 Compare November 10, 2025 10:10

foursixnine reviewed Nov 10, 2025

View reviewed changes

d3flex force-pushed the feat/gitea_amqp_handler branch from 63cbbc2 to b71f160 Compare November 10, 2025 12:14

Martchus requested changes Nov 10, 2025

View reviewed changes

d3flex force-pushed the feat/gitea_amqp_handler branch 6 times, most recently from 3c39842 to 5a8e15f Compare November 12, 2025 11:55

d3flex force-pushed the feat/gitea_amqp_handler branch from 5a8e15f to 95c8adf Compare November 12, 2025 12:05

d3flex force-pushed the feat/gitea_amqp_handler branch from 95c8adf to b5908a4 Compare November 12, 2025 12:21

d3flex force-pushed the feat/gitea_amqp_handler branch from 9db60f7 to 79fbfee Compare November 13, 2025 15:16

d3flex force-pushed the feat/gitea_amqp_handler branch from 79fbfee to f4a14db Compare November 13, 2025 15:50

foursixnine reviewed Nov 13, 2025

View reviewed changes

foursixnine reviewed Nov 14, 2025

View reviewed changes

	log.debug(" %s - %s", method.routing_key, body.decode())
	log.debug("%s - %s", method.routing_key, body.decode())

	log.info("%s - Received gitea review request message", method.routing_key)
	log.info("%s - Received Gitea review request message", method.routing_key)

	def qem_mock_get_incident_settings() -> Any:
	def mock_get_incident_settings() -> Any:

	def qem_mock_get_incident_settings() -> Any:
	def mock_qem_get_incident_settings() -> Any:

	A json-encoded bytes object of gitea review request message
	A JSON-encoded bytes object of a Gitea review request message

		# Check fixtures in tests/fixtures/qembot_mocks.py
		# imported from conftest.py

checks for gitea PRs based on AMQP messages #272

Are you sure you want to change the base?

checks for gitea PRs based on AMQP messages #272

Uh oh!

Conversation

d3flex commented Nov 10, 2025

Uh oh!

d3flex commented Nov 10, 2025

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!

Martchus left a comment

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!

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!

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!

d3flex commented Nov 10, 2025

Uh oh!

codecov bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

foursixnine Nov 13, 2025 • 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!

foursixnine Nov 14, 2025 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 12, 2025 •

edited

Loading

foursixnine Nov 13, 2025 •

edited

Loading

foursixnine Nov 14, 2025 •

edited

Loading