Role `junit2json` and its infrastructure #179

mvk · 2025-08-21T10:04:23Z

implements:

filter plugin junuit2obj converts a junit xml report to json report of similar structure
filter plugin reportsmerger merges several json reports into 1 big report, updating aggregate metrics (counters, times)
role junit2json the role to be used to convert and merge junit xml reports
unit tests for both plugins

eifrach · 2025-08-21T11:02:58Z

playbooks/infra/reporting/roles/junit2json/tasks/main.yml

+- name: Expand the input list to list of existing files
+  ansible.builtin.include_tasks:
+    file: expand.yml
+  loop: "{{ junit2_input_reports_list }}"


this is not a list - in the default.yml it set as a string

This variable is a list of str, defined here: playbooks/infra/reporting/roles/junit2json/meta/argument_specs.yml, the 1st variable.

thats not a var location - it's metadata
under ./default/main.yml the var is defined as str

eifrach · 2025-08-21T11:03:10Z

playbooks/infra/reporting/roles/junit2json/tasks/main.yml

+    file: expand.yml
+  loop: "{{ junit2_input_reports_list }}"
+  loop_control:
+    loop_var: file_name


where is the var coming from ?

also change the name -- file_name is too generic

junit2_input_reports_list is role's variable (refer to defaults/main.yml and meta/argument_spec.yml),

loop_var is how I set the name of the variable instead of item (for the loop, to be used inside expand.yml

eifrach

This is very complex - there is a lot of naming issues / redandant tasks and vars

please go over the comment - and try to make it simpler and shorter

we can skip / merge quite a few steps here
I'll review it once your done

eifrach · 2025-08-21T11:04:09Z

playbooks/infra/reporting/roles/junit2json/tasks/main.yml

+    file: expand.yml
+  loop: "{{ junit2_input_reports_list }}"
+  loop_control:
+    loop_var: file_name


also change the name -- file_name is too generic

eifrach · 2025-08-21T11:05:42Z

playbooks/infra/reporting/roles/junit2json/tasks/expand.yml

+- name: Collect file_name stat
+  ansible.builtin.stat:
+    path: "{{ file_name }}"
+  register: _junit2json_path_item_stat


the name should include the task name or the file name
example

Suggested change

register: _junit2json_path_item_stat

register: _expand_file_stat

I want the file_name to be as generic, so that I can do ansible.builtin.import_tasks from this role in another role.

eifrach · 2025-08-21T11:06:36Z

playbooks/infra/reporting/roles/junit2json/tasks/expand.yml

+      - not(_junit2json_path_item_stat.stat.isdir | default(false))
+      - _junit2json_path_item_stat.stat.exists | default(false)


no need for default

Suggested change

- not(_junit2json_path_item_stat.stat.isdir | default(false))

- _junit2json_path_item_stat.stat.exists | default(false)

- not(_junit2json_path_item_stat.stat.isdir

- _junit2json_path_item_stat.stat.exists

eifrach · 2025-08-21T11:07:39Z

playbooks/infra/reporting/roles/junit2json/tasks/expand.yml

+  ansible.builtin.set_fact:
+    junit2_reports_list: "{{ junit2_reports_list + [file_name] }}"
+  when:
+    - _junit2json_path_item_stat.stat.exists | default(false)


Suggested change

- _junit2json_path_item_stat.stat.exists | default(false)

- _junit2json_path_item_stat.stat.exists

eifrach · 2025-08-21T11:08:39Z

playbooks/infra/reporting/roles/junit2json/tasks/expand.yml

+
+- name: Update junit2_reports_list with a JUnit XML report item
+  ansible.builtin.set_fact:
+    junit2_reports_list: "{{ junit2_reports_list + [file_name] }}"


you initialize this as empty value and then you trying to add empty with the file_name?

lets do the initialisation here and remove it from main

@eifrach because I am testing this role in several calls to the role, the list must be reset this list each time the code is in tasks/main.yml. (Otherwise this list will be updated each run)
It is possible to put it deeper than tasks/main.yml, but I think tasks/main.yml` is more readable.
WDYT?

you don't need to change you're code to make you test work
it should be the other way around

eifrach · 2025-08-21T11:14:09Z

playbooks/infra/reporting/roles/junit2json/tasks/convert.yml

+- name: Ensure junit2_output_dir is created
+  ansible.builtin.include_tasks:
+    file: ensure-dir.yml
+  loop:
+    - "{{ junit2_output_dir }}"
+  loop_control:
+    loop_var: folder_path


why is this a loop ? are we creating more than one folder?

also if you only create the folder once no need to put it in a new file / playboolk

@eifrach 👍 thank you, good catch, it is a left-over

eifrach · 2025-08-21T11:15:38Z

playbooks/infra/reporting/roles/junit2json/tasks/convert.yml

+    content: "{{ junit2_result_data | junit2obj | to_nice_json(indent=2) }}"
+    dest: "{{ junit2_output_report_path }}"
+    mode: '0644'
+  when: not junit2_out_str | bool


I don't get this please explain

2 use cases:

with the filter to_nice_json(indent=2) creates a serializable JSON data, is easy to read and compare, load and merge.

without the filter, a single string is created. It is more convenient for dumping all of it in a template.

eifrach · 2025-08-21T11:15:43Z

playbooks/infra/reporting/roles/junit2json/tasks/convert.yml

+    content: "{{ junit2_result_data | junit2obj }}"
+    dest: "{{ junit2_output_report_path }}"
+    mode: '0644'
+  when: junit2_out_str | bool


eifrach · 2025-08-21T11:16:17Z

playbooks/infra/reporting/roles/junit2json/tasks/main.yml

+  ansible.builtin.include_tasks:
+    file: merge.yml
+  when:
+    - junit2_do_merge


there are cases when we don't merge it ?

if so when does it change?

during tests mainly.

you don't need to test a case we don't use

eifrach · 2025-08-21T11:16:47Z

playbooks/infra/reporting/roles/junit2json/tasks/merge.yml

+- name: Load dummy variables configuration
+  ansible.builtin.include_vars:
+    file: dummy_variables.yml


the data inside test reports sometimes contains parts of jinja code, Please refer to this file:
https://github.com/mvk/eco-ci-cd/blob/role-junit2obj/playbooks/infra/reporting/roles/junit2json/doc/dummy_variables.md

The name of the role and the file are not related to what it's for

can you explain why we shouldn't create a simple python script to do this instead of this complex steps ?

shaior · 2025-08-21T11:48:17Z

General comment - you added a role under playbooks/infra/reporting but this path is not included under roles_path in ansible.cfg.

Also, it's very difficult to review this since it's too long, I would split it into several much shorter PRs with proper documentation.

openshift-ci · 2025-08-21T12:54:48Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from eifrach. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mvk · 2025-08-21T21:06:13Z

General comment - you added a role under playbooks/infra/reporting but this path is not included under roles_path in ansible.cfg.

Also, it's very difficult to review this since it's too long, I would split it into several much shorter PRs with proper documentation.

@shaior, I referred to storing-and-finding-roles, namely:

in a directory called roles/, relative to the playbook file

this means we don't have to update ansible.cfg.

mvk · 2025-08-21T21:07:42Z

General comment - you added a role under playbooks/infra/reporting but this path is not included under roles_path in ansible.cfg.

Also, it's very difficult to review this since it's too long, I would split it into several much shorter PRs with proper documentation.

@shaior

splitting this one into several commits now, will comment on it, then I will break it down to several PRs by those commits.

Signed-off-by: Maxim Kovgan <makovgan@redhat.com>

- code - tests + data Signed-off-by: Maxim Kovgan <makovgan@redhat.com>

Signed-off-by: Maxim Kovgan <makovgan@redhat.com>

eifrach · 2025-08-25T08:29:06Z

I feel this is very complex and we can do it in a simple python script
too many workarounds and supporting documentation just to make it work is very bad practice
also I think if you use Ansible builtin feature for ansible.utils.from_xml it will solve a lot of issues and workarounds

let's talk to the team and re evaluate this

cc @shaior @kononovn

openshift-ci bot requested review from cplacani and kononovn August 21, 2025 10:04

$eifrach$

eifrach reviewed Aug 21, 2025

View reviewed changes

$eifrach$

eifrach suggested changes Aug 21, 2025

View reviewed changes

openshift-ci bot assigned eifrach Aug 21, 2025

mvk force-pushed the role-junit2obj branch from efc909a to ed699c5 Compare August 21, 2025 12:54

mvk force-pushed the role-junit2obj branch 4 times, most recently from ee0df99 to 948ee49 Compare August 21, 2025 20:55

mvk force-pushed the role-junit2obj branch 4 times, most recently from d9ddf94 to 19bffd2 Compare August 23, 2025 00:30

mvk added 4 commits August 24, 2025 19:27

## adding plugin filter junit2obj + tests

8b0afd8

Signed-off-by: Maxim Kovgan <makovgan@redhat.com>

## adding plugin filter reportsmerger + tests

c62c61a

- code - tests + data Signed-off-by: Maxim Kovgan <makovgan@redhat.com>

## adding role junit2json

f08ecc3

Signed-off-by: Maxim Kovgan <makovgan@redhat.com>

## adding role junit2json tests

7b6d8b1

Signed-off-by: Maxim Kovgan <makovgan@redhat.com>

mvk force-pushed the role-junit2obj branch from 19bffd2 to 7b6d8b1 Compare August 24, 2025 16:30

	register: _junit2json_path_item_stat
	register: _expand_file_stat

		- not(_junit2json_path_item_stat.stat.isdir \| default(false))
		- _junit2json_path_item_stat.stat.exists \| default(false)

Role junit2json and its infrastructure #179

Are you sure you want to change the base?

Role junit2json and its infrastructure #179

Uh oh!

Conversation

mvk commented Aug 21, 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!

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!

eifrach 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!

mvk Aug 21, 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!

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!

shaior commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Aug 21, 2025

Uh oh!

mvk commented Aug 21, 2025

Uh oh!

mvk commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Role `junit2json` and its infrastructure #179

Role `junit2json` and its infrastructure #179

$@eifrach$ eifrach left a comment

mvk Aug 21, 2025 •

edited

Loading

shaior commented Aug 21, 2025 •

edited

Loading

mvk commented Aug 21, 2025 •

edited

Loading

eifrach commented Aug 25, 2025 •

edited

Loading