Skip to content

fedora-review: allow skipping checks #34

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

Open
FrostyX wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fedora-review: allow skipping checks #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
807f9aa
fedora-review: allow skipping checks
FrostyX Apr 7, 2026
6a55042
fedora-review: drop temporary workaround for missing viewer.html
FrostyX Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions plans/fedora-review/main.fmf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,11 @@ provision:
discover:
how: fmf
test: /tests/fedora-review

# TODO Remove the prepare section once this PR
# https://forge.fedoraproject.org/packaging/FedoraReview/pulls/551
# is merged and released
prepare:
how: artifact
provide:
- copr.build:10883634:fedora-44-x86_64
2 changes: 2 additions & 0 deletions tests/fedora-review/main.fmf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
require:
- koji
- git
- python3-ruamel-yaml
- python3-tomli-w
test: python3 ./distgit-prepare.py
adjust:
# TODO: For now this is only available for distgit commits
Expand Down
112 changes: 88 additions & 24 deletions tests/fedora-review/run-fedora-review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@
from enum import Enum
import json
import yaml
import utils
import tomli_w

CI_CONFIG_SECTION = "fedora-review"

# Expose these to the users
FEDORA_REVIEW_RESULTS = [
Expand All @@ -27,7 +31,7 @@ class Result(Enum):
PASS = "pass"


def dump_results_yaml(issues: int):
def dump_results_yaml(issues: int, skipped: int):
"""
https://tmt.readthedocs.io/en/stable/spec/results.html
"""
Expand All @@ -36,8 +40,11 @@ def dump_results_yaml(issues: int):
{
"name": "/",
"result": result.value,
"note": [f"{issues} issues"],
"log": ["viewer.html"] + FEDORA_REVIEW_RESULTS,
"note": [
f"{skipped} skipped",
f"{issues} issues",
],
"log": ["viewer.html", "fedora-review.toml"] + FEDORA_REVIEW_RESULTS,
}
]
path = os.path.join(os.environ.get("TMT_TEST_DATA"), "results.yaml")
Expand Down Expand Up @@ -72,20 +79,17 @@ def copy_viewer_html():
shutil.copy(viewer, Path(os.environ["TMT_TEST_DATA"]) / viewer)


def copy_data_into_data():
def copy_mock_fedora_ci_toml():
"""
There is a weird bug that we discovered with @LecrisUT. For some reason,
when a plan has `result: custom`, the `viewer.html` stops rendering in
Testing Farm. It is because for some reason, Oculus starts looking for it
in `data/data/viewer.html` instead of just `data/viewer.html`.
This is IMHO a bug but either way, until it gets resolved, we can copy the
data there as well.
See https://gitlab.com/testing-farm/general/-/work_items/111
Copy a mock fedora-ci.toml to the plan data directory
This is only for development purposes. In production a package either has
a fedora-ci.toml configuration in its repository or it doesn't. Either way,
we don't want to copy it from anywhere else.
"""
shutil.copytree(
Path(os.environ["TMT_TEST_DATA"]),
Path(os.environ["TMT_TEST_DATA"]) / "data",
)
filename = "fedora-ci.toml"
print(f"Copying {filename} to the plan data")
dst = Path(os.environ["TMT_PLAN_DATA"]) / "dist-git" / filename
shutil.copy(filename, dst)


def fedora_review(spec_file, workdir):
Expand All @@ -96,9 +100,20 @@ def fedora_review(spec_file, workdir):
env["REVIEW_NO_MOCKGROUP_CHECK"] = "true"

name = Path(spec_file).stem
cmd = ["fedora-review", "--prebuilt", "-n", name]
config = str(args.workdir / "fedora-review.toml")
cmd = ["fedora-review", "--config", config, "--prebuilt", "-n", name]
print(f"Running: {" ".join(cmd)}")
subprocess.run(cmd, cwd=workdir, env=env, check=True)
proc = subprocess.run(
cmd,
cwd=workdir,
env=env,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
print(proc.stdout.decode("utf-8"))
print(proc.stderr.decode("utf-8"))
if proc.returncode:
raise RuntimeError("The fedora-review command failed")
Comment on lines -101 to +116

@LecrisUT LecrisUT Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why this change? It seems to do the same thing as before, but with extra steps. You could just try-catch if what you want a different exception message.

@FrostyX FrostyX Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because with the previous version, the logs show

Test invocation process spawned with pid 1713057.
        stdout: INFO: Processing local files: hatch
        stdout: INFO: Getting .spec and .srpm Urls from : Local files in /var/tmp/tmt/run-021/plans/fedora-review/data
        stdout: INFO:   --> SRPM url: file:///var/tmp/tmt/run-021/plans/fedora-review/data/hatch-1.18.0-1.eln159.src.rpm
        stdout: INFO:   --> Spec url: file:///var/tmp/tmt/run-021/plans/fedora-review/data/hatch.spec
        stdout: INFO: Using review directory: /var/tmp/tmt/run-021/plans/fedora-review/data/review-hatch
        stdout: WARNING: No disttag found in prebuilt packages
        stdout: INFO: Use --define DISTTAG to set proper dist. e. g. --define DISTTAG fc21.
        stdout: ERROR: 'No disttag in package and no DISTTAG flag. Use --define DISTTAG to set proper dist e. g., --define DISTTAG=fc21.' (logs in /root/.cache/fedora-review.log)
        stdout: Copying fedora-review.toml to the test results
        stdout: Skipping these checks: ['CheckNoNameConflict', 'CheckLicensInDoc', 'CheckLicenseField']
        stdout: Running: fedora-review --config /var/tmp/tmt/run-021/plans/fedora-review/data/fedora-review.toml --prebuilt -n hatch
        stdout: Traceback (most recent call last):
        stdout:   File "/var/tmp/tmt/run-021/plans/fedora-review/discover/default-0/tests/tests/fedora-review/./run-fedora-review.py", line 238, in <module>
        stdout:     main(args)
        stdout:     ~~~~^^^^^^
        stdout:   File "/var/tmp/tmt/run-021/plans/fedora-review/discover/default-0/tests/tests/fedora-review/./run-fedora-review.py", line 196, in main
        stdout:     review = fedora_review(args.spec_file, args.workdir)
        stdout:   File "/var/tmp/tmt/run-021/plans/fedora-review/discover/default-0/tests/tests/fedora-review/./run-fedora-review.py", line 117, in fedora_review
        stdout:     subprocess.run(cmd, cwd=workdir, env=env, check=True)
        stdout:     ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        stdout:   File "/usr/lib64/python3.14/subprocess.py", line 577, in run
        stdout:     raise CalledProcessError(retcode, process.args,
        stdout:                              output=stdout, stderr=stderr)
        stdout: subprocess.CalledProcessError: Command '['fedora-review', '--config', '/var/tmp/tmt/run-021/plans/fedora-review/data/fedora-review.toml', '--prebuilt', '-n', 'hatch']' returned non-zero exit status 1.
        stdout: Shared connection to 127.0.0.1 closed.^M
Command returned '1' (failure).

with the ugly traceback, and now the logs show just:

Test invocation process spawned with pid 1685990.
        stdout: The fedora-review command failed
        stdout: Copying fedora-review.toml to the test results
        stdout: Skipping these checks: ['CheckNoNameConflict', 'CheckLicensInDoc', 'CheckLicenseField']
        stdout: Running: fedora-review --config /var/tmp/tmt/run-020/plans/fedora-review/data/fedora-review.toml --prebuilt -n hatch
        stdout:
        stdout: INFO: Processing local files: hatch
        stdout: INFO: Getting .spec and .srpm Urls from : Local files in /var/tmp/tmt/run-020/plans/fedora-review/data
        stdout: INFO:   --> SRPM url: file:///var/tmp/tmt/run-020/plans/fedora-review/data/hatch-1.18.0-1.eln159.src.rpm
        stdout: INFO:   --> Spec url: file:///var/tmp/tmt/run-020/plans/fedora-review/data/hatch.spec
        stdout: INFO: Using review directory: /var/tmp/tmt/run-020/plans/fedora-review/data/review-hatch
        stdout: WARNING: No disttag found in prebuilt packages
        stdout: INFO: Use --define DISTTAG to set proper dist. e. g. --define DISTTAG fc21.
        stdout: ERROR: 'No disttag in package and no DISTTAG flag. Use --define DISTTAG to set proper dist e. g., --define DISTTAG=fc21.' (logs in /root/.cache/fedora-review.log)
        stdout:
        stdout: Shared connection to 127.0.0.1 closed.^M
Command returned '1' (failure).

@LecrisUT LecrisUT Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah CalledProcessError != RuntimeError. Try the pattern in rmdepcheck instead

@FrostyX FrostyX Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's also somehow weird

Test invocation process spawned with pid 3705330.
        stdout: INFO: Processing local files: hatch
        stdout: INFO: Getting .spec and .srpm Urls from : Local files in /var/tmp/tmt/run-030/plans/fedora-review/data
        stdout: INFO:   --> SRPM url: file:///var/tmp/tmt/run-030/plans/fedora-review/data/hatch-1.18.0-1.eln159.src.rpm
        stdout: INFO:   --> Spec url: file:///var/tmp/tmt/run-030/plans/fedora-review/data/hatch.spec
        stdout: INFO: Using review directory: /var/tmp/tmt/run-030/plans/fedora-review/data/review-hatch
        stdout: WARNING: I can't remove check: Check1
        stdout: WARNING: I can't remove check: FooBarCheck
        stdout: WARNING: I can't remove check: Baz
        stdout: WARNING: No disttag found in prebuilt packages
        stdout: INFO: Use --define DISTTAG to set proper dist. e. g. --define DISTTAG fc21.
        stdout: ERROR: 'No disttag in package and no DISTTAG flag. Use --define DISTTAG to set proper dist e. g., --define DISTTAG=fc21.' (logs in /root/.cache/fedora-review.log)
        stdout: Copying fedora-ci.toml to the plan data
        stdout: Copying fedora-review.toml to the test results
        stdout: Skipping these checks: ['CheckNoNameConflict', 'CheckLicensInDoc', 'CheckLicenseField', 'Check1', 'FooBarCheck', 'Baz', 'CheckPythonBuildRequires']
        stdout: Running: fedora-review --config /var/tmp/tmt/run-030/plans/fedora-review/data/fedora-review.toml --prebuilt --name hatch
        stdout: The fedora-review command failed

See how the "Running: fedora-review" line is after the actual failure?


path = os.path.join(workdir, "review-" + name, "review.json")
if not os.path.exists(path):
Expand All @@ -110,9 +125,46 @@ def fedora_review(spec_file, workdir):
return review


def count_issues(review):
issues = review.get("issues", [])
return len(issues)
def skip_checks(config):
skip_for_all = [
# A package with this name obviously already exists in the Fedora
# repositories and this is that package. Check for a name conflict only
# makes sense during the initial Package Review Process, but it does't
# make any sense for CI on existing packages.
"CheckNoNameConflict",
# The licensecheck implementation within the `fedora-review` tool is
# not up to modern standards and produces far to many false-positives
# which would be too annoying for our users. We discussed this with
# @msuchy and agreed that it would be better to have a dedicate service
# for checking licenses. It should be based around ScanCode Toolkit,
# FOSSology, or anything that succeeds them.
"CheckLicensInDoc",
"CheckLicenseField",
]
Comment on lines +129 to +143

@LecrisUT LecrisUT Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we put these in a fedora-review.toml file that is merged with possible data from the user? It would make it easier for the user to reference the format for also.

@FrostyX FrostyX Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So we would run something like fedora-review --config fedora-review-generic.toml --config fedora-review-from-user.toml ...?

I am not opposed to it but it would require some additional changes on the fedora-review side so if you don't mind, I'd rather do it in a follow-up PR later. And for now maybe just document som how to exclude tests? Do you have a recommendation where?

skip_for_package = []
if exclude := config.get("exclude"):
skip_for_package = [x.strip() for x in exclude.split(",")]
skip_for_package = [x for x in skip_for_package if x]
return skip_for_all + skip_for_package


def parse_fedora_review_toml(workdir: Path):
"""
Parse the fedora-review.toml out of the fedora-ci.toml
"""
dist_git_path = args.workdir / "dist-git"
if config := utils.get_config(dist_git_path, CI_CONFIG_SECTION):
return config["toml"]

@LecrisUT LecrisUT Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can drop the toml part on this. rpmlint had it because it has 2 distinct helper files rpmlintrc and rpmint.toml, but for this one it is only one.

Then this is just a one-liner

return utils.get_config(args.workdir / "dist-git", CI_CONFIG_SECTION) or {}

@FrostyX FrostyX Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think we can because the config is a <class 'dict'> and looks like this

{'toml': {'mock_config': 'fedora-43-x86_64', 'cache': False, 'verbose': True, 'checksum': 'sha256', 'use_colors': True, 'exclude': 'Check1,FooBarCheck,Baz,CheckPythonBuildRequires'}}

so we need to go through the toml key first. Unless you want to bypass it on the utils level.

return {}


def dump_fedora_review_config(fedora_review_config):
name = "fedora-review.toml"
path: Path = args.workdir / name
with path.open("wb") as fp:
tomli_w.dump(fedora_review_config, fp)
print(f"Copying {name} to the test results")
shutil.copy(path, Path(os.environ["TMT_TEST_DATA"]) / name)
Comment on lines +151 to +167

@LecrisUT LecrisUT Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm thinking if these should be in the distgit-prepare.py. Reasoning being that we would have a different logic for copr, with different defaults as well.

@FrostyX FrostyX Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I noticed that distgit-prepare.py does something related

tmt-plans/tests/rpmlint/distgit-prepare.py

Lines 26 to 39 in 8c265cf

def set_config_files(config: dict[str, Any], args: argparse.Namespace) -> None:
if rc_content := config.get("rc"):
rc_content: str
rc_file: Path = args.workdir / "rpmlintrc"
rc_file.write_text(rc_content)
with args.env_file.open("a") as f:
f.write(f"RPMLINT_RC_FILE={rc_file}\n")
if toml_content := config.get("toml"):
toml_content: dict[str, Any]
toml_file: Path = args.workdir / "rpmlint.toml"
with toml_file.open("wb") as f:
tomli_w.dump(toml_content, f)
with args.env_file.open("a") as f:
f.write(f"RPMLINT_TOML_FILE={toml_file}\n")

but I was thinking the exact opposite. I see distgit-prepare.py as a generic helper script that shouldn't be specific to any one test but rather useful for all of them. So IMHO there shouldn't be any fedora-review or rpmlint configuration handling inside that script and it should rather be done in the tests.

If there is some re-usable piece of code that all the tests should use, it should probably be in utils.



def main(args: argparse.Namespace) -> None:
Expand All @@ -129,14 +181,26 @@ def main(args: argparse.Namespace) -> None:
# we just need to copy the .spec next to them
shutil.copy(args.spec_file, args.workdir)

# Uncomment if needed for development purposes
# copy_mock_fedora_ci_toml()

# Parse the `fedora-review config` aout of the `fedora-ci.toml`, update
# the list of excluded checks and save it as `fedora-review.toml`.
config = parse_fedora_review_toml(args.workdir)
skip = skip_checks(config)
config["exclude"] = ",".join(skip)
dump_fedora_review_config(config)
print(f"Skipping these checks: {skip}")

review = fedora_review(args.spec_file, args.workdir)
issues = count_issues(review)
dump_results_yaml(issues)
issues = review.get("issues", [])

dump_results_yaml(len(issues), len(skip))
copy_fedora_review_results(args.spec_file, args.workdir)
copy_viewer_html()
copy_data_into_data()

print(f"Found {issues} issues")
print(f"Skipped {len(skip)} issues")
print(f"Found {len(issues)} issues")
if issues:
sys.exit(1)

Expand Down
4 changes: 2 additions & 2 deletions tests/fedora-review/viewer.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ <h2>
} from "https://cdn.jsdelivr.net/gh/lit/dist@2/core/lit-core.min.js";

function getReviewIssues(report) {
return report["issues"];
return report["issues"] ?? [];
}

function renderIssue(issue) {
Expand All @@ -87,7 +87,7 @@ <h2>
const issues = getReviewIssues(report);
const summary = [];

if (issues?.length === 0) {
if (issues.length === 0) {
summary.push(html`<div class="result-OK">No issues were found</div>`);
} else {
issues.forEach((issue) => {
Expand Down