Skip to content

Add sub command for product increment approval #222

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add sub command for product increment approval #222

wants to merge 1 commit into from
+322 −1

Conversation

Martchus
Copy link
Contributor

@Martchus
Copy link
Contributor Author

Martchus commented Jul 17, 2025 •
edited
Loading

I added the "not-ready" label because this needs os-autoinst/openQA#6592.

I tested this locally against my openQA instance and IBS, e.g. via:

$ ./bot-ng.py --debug --dry -t not-secret -i 'http://[::1]:9526' increment-approve --flavor Foo-Bar-Incidents
2025-07-17 13:59:42 INFO     Checking for product increment requests to be reviewed by qam-openqa on SUSE:SLFO:Products:SLES:15.99:TEST
2025-07-17 13:59:43 INFO     Skipping approval, no relevant requests in states new/review

$ ./bot-ng.py --debug --dry -t not-secret -i 'http://[::1]:9526' increment-approve --accepted --flavor Foo-Bar-Incidents
2025-07-17 13:57:57 INFO     Checking for product increment requests to be reviewed by qam-openqa on SUSE:SLFO:Products:SLES:15.99:TEST
2025-07-17 13:58:03 INFO     Found request 380875
2025-07-17 13:58:03 INFO     Checking openQA job results
2025-07-17 13:58:03 DEBUG    Job statistics:
{'done': {'incomplete': {'job_ids': ['5059'],
                         'latest_job_result': 'incomplete',
                         'latest_job_state': 'done',
                         'scheduled_product_ids': ['330']},
          'softfailed': {'job_ids': ['5062'],
                         'latest_job_result': 'softfailed',
                         'latest_job_state': 'done',
                         'scheduled_product_ids': [None]}}}
2025-07-17 13:58:03 INFO     Not approving for the following reasons:
The following openQA jobs ened up with result 'incomplete':
 - http://[::1]:9526/tests/5059

I didn't test the approval for real but it is simply using the function that the bot also uses for other approvals so it most likely works.

@Martchus Martchus mentioned this pull request Jul 17, 2025
Draft
@Martchus Martchus force-pushed the approve-increment branch from 623738e to 4a523df Compare July 17, 2025 16:46
okurz
okurz requested changes Jul 18, 2025
View reviewed changes
openqabot/args.py
required=False,
type=str,
default="sle",
help="The DISTRI parameter of relevant scheuled products on openQA",
Copy link
Member

Choose a reason for hiding this comment

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

s/scheuled/scheduled/

openqabot/args.py
required=False,
type=str,
default="15.99",
help="The VERSION parameter of relevant scheuled products on openQA",
Copy link
Member

Choose a reason for hiding this comment

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

Same here and in next arguments

openqabot/incrementapprover.py
relevant_request = request
break
else:
log.info("Checking specified request %i", args.request_id)
Copy link
Member

Choose a reason for hiding this comment

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

This could be debug as it only states what the process is doing, not any result like the other info calls you introduced

openqabot/incrementapprover.py
+ "/".join(relevant_states)
)
return 0
log.info("Found request %s", relevant_request.id)
Copy link
Member

Choose a reason for hiding this comment

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

IMHO debug, not info

openqabot/incrementapprover.py
log.info("Found request %s", relevant_request.id)

# query openQA for job results
log.info("Checking openQA job results")
Copy link
Member

Choose a reason for hiding this comment

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

Same here

openqabot/incrementapprover.py
return 0
log.info("Found request %s", relevant_request.id)

# query openQA for job results
Copy link
Member

Choose a reason for hiding this comment

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

Redundant as the log message already states that

openqabot/incrementapprover.py
res = self.client.get_scheduled_product_stats(params)
log.debug("Job statistics:\n%s", pformat(res))

# return earily if there are no jobs or jobs are still pending
Copy link
Member

Choose a reason for hiding this comment

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

Typo

openqabot/incrementapprover.py
map(lambda id: f" - {openqa_url}/tests/{id}", info["job_ids"])
)
reasons_to_disapprove.append(
f"The following openQA jobs ened up with result '{result}':\n{job_list}"
Copy link
Member

Choose a reason for hiding this comment

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

Typo "ened"

b10n1k
b10n1k reviewed Jul 18, 2025
View reviewed changes
openqabot/args.py
"--obs-project",
required=False,
type=str,
default="SUSE:SLFO:Products:SLES:15.99:TEST",
Copy link

Choose a reason for hiding this comment

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

I found it a bit weird that the parameter is implies the build.opesuse.org but the default project cant be found there. I guess just --project would do better fit. wdyt?

b10n1k
b10n1k reviewed Jul 18, 2025
View reviewed changes
openqabot/args.py
cmdincrementapprove.add_argument(
"--accepted",
action="store_true",
help="Consider accepted requests/reviews as well",
Copy link

Choose a reason for hiding this comment

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

not clear to me what this does as parameter. I guess I am missing a lot more but this is the first place I would look to find out. How does it fit in the increment-approve or how is different without?

b10n1k
b10n1k reviewed Jul 18, 2025
View reviewed changes
tests/test_incrementapprover.py
def test_not_ok_openqa_jobs(caplog, fake_not_ok_jobs, monkeypatch):
run_approver(caplog, monkeypatch)
last_message = [x[-1] for x in caplog.record_tuples][-1]
assert "The following openQA jobs ened up with result 'failed'" in last_message
Copy link

Choose a reason for hiding this comment

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

typo: s/ened/end/

b10n1k
b10n1k requested changes Jul 18, 2025
View reviewed changes
Copy link

@b10n1k b10n1k left a comment

Choose a reason for hiding this comment

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

Everything looks good but I have a small observation. tests/test_incrementapprover.py has some tests which I would call them unit tests. Although I found the descriptions more general. For example test_not_ok_openqa_jobs doesnt show what should I expect from this test in regard of the behaviour but only the focus on the test, which is failing jobs. Do you think you could provide some alternatives which explain what are the expectation from IncrementApprover?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b10n1k b10n1k b10n1k requested changes

@okurz okurz okurz requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
not-ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Martchus @b10n1k @okurz