Skip to content

[Draft] Small improvement on sanity checks per MI repository provided on the autogenerated JSON #1851

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

Draft
srbarrios wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[Draft] Small improvement on sanity checks per MI repository provided on the autogenerated JSON #1851

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0aa72ef
First small iterarion
srbarrios Jan 14, 2026
5ea2a60
Second iteration including Debian-like logic
srbarrios Jan 14, 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
45 changes: 42 additions & 3 deletions jenkins_pipelines/scripts/json_generator/maintenance_json_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import json
import requests
import logging
import re

from ibs_osc_client import IbsOscClient
from repository_versions import nodes_by_version
Expand Down Expand Up @@ -45,11 +46,49 @@ def clean_mi_ids(mi_ids: list[str]) -> set[str]:
return { id.replace(',', '') for id in mi_ids }

@cache
def create_url(mi_id: str, suffix: str) -> str:
def create_url(mi_id: str, suffix: str) -> str | None:
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The return type of create_url has changed from returning an empty string "" for invalid URLs to returning None. However, the existing test at jenkins_pipelines/scripts/tests/test_maintenance_json_generator.py:99 still expects an empty string ("") for invalid cases. This test will fail because it checks 'expected_url: str = ... if valid else ""' and then asserts equality with the result. The test needs to be updated to expect None instead of "" for invalid URLs.

Copilot uses AI. Check for mistakes.
"""
Build the maintenance URL for the given MI and suffix.
Validates if it's a valid RPM repo (contains .repo) or Debian repo (contains Release/InRelease).
Returns the directory URL if valid, None otherwise.
Comment on lines +52 to +53
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The docstring states "Validates if it's a valid RPM repo (contains .repo) or Debian repo (contains Release/InRelease)". However, this is slightly misleading as the function doesn't just validate - it also constructs and returns the URL. Consider updating the docstring to clarify that the function both validates and returns the URL, or returns None if validation fails. For example: "Validates if the URL points to a valid RPM or Debian repository and returns the URL if valid, None otherwise."

Suggested change
Validates if it's a valid RPM repo (contains .repo) or Debian repo (contains Release/InRelease).
Returns the directory URL if valid, None otherwise.
Validates if the URL points to a valid RPM repository (contains a .repo file) or Debian repository
(contains Release/InRelease), and returns the URL if valid, None otherwise.

Copilot uses AI. Check for mistakes.
"""
url = f"{IBS_MAINTENANCE_URL_PREFIX}{mi_id}{suffix}"

res: requests.Response = requests.get(url, timeout=6)
return url if res.ok else ""
try:
res: requests.Response = requests.get(url, timeout=6)
res.raise_for_status()
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The code now calls 'res.raise_for_status()' on line 59, which is the correct requests library pattern. However, the test mock at jenkins_pipelines/scripts/tests/mock_response.py:18-19 currently always raises HTTPError regardless of the response status. This means all test cases will fail because even successful (200) responses will raise an exception. The MockResponse.raise_for_status() method should only raise HTTPError when status_code indicates an error (4xx, 5xx), not for successful responses.

Copilot uses AI. Check for mistakes.
except requests.RequestException as e:
logging.error(f"Error requesting: {url}; {e}")
Copy link
Copy Markdown
Contributor

@NamelessOne91 NamelessOne91 Jan 19, 2026

Choose a reason for hiding this comment

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

As pointed out by Kostas, this will log all 404s that we get.
Since they tend to be quite a lot, we may want to skip logging here.

Copy link
Copy Markdown
Member Author

@srbarrios srbarrios Jan 19, 2026

Choose a reason for hiding this comment

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

Why we get 404 on repos that we expect to be valid?

return None

body = res.text or ""
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The new create_url function now reads 'res.text' on line 64 to get the response body for validation. However, the test mock at jenkins_pipelines/scripts/tests/mock_response.py:9-13 only defines 'content' attribute, not 'text'. The tests will fail with AttributeError when trying to access res.text. The MockResponse class needs to be updated to include a 'text' property that returns the content.

Copilot uses AI. Check for mistakes.

# --- Strategy 1: RPM Repository Check ---
# If the URL itself points directly to a .repo file
if url.lower().endswith('.repo'):
return url.rsplit('/', 1)[0] + '/'

Comment on lines +67 to +70
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

This logic appears problematic. If the URL itself ends with '.repo', it means the URL was constructed to point directly to a .repo file (which seems unlikely based on how create_url is called with MI IDs and suffixes). The code then returns the parent directory by stripping the filename, but this parent directory hasn't been validated to exist. Additionally, this check happens before checking the response body, so even if the URL returns a 404, it would still return the parent directory. Consider moving this check after validating the response, or removing it if URLs are never expected to point directly to .repo files.

Suggested change
# If the URL itself points directly to a .repo file
if url.lower().endswith('.repo'):
return url.rsplit('/', 1)[0] + '/'

Copilot uses AI. Check for mistakes.
# Search for links to .repo files in the HTML body
# Matches: href="something.repo"
rpm_regex = re.compile(r'href=["\'](?P<link>[^"\']+?\.repo)\b["\']', re.IGNORECASE)
if rpm_regex.search(body):
return url

# Fallback for plain text listings of .repo files
token_repo_re = re.compile(r'(?P<link>https?://[^\s"\'<>]+?\.repo\b)|(?P<link2>[^\s"\'<>]+?\.repo\b)', re.IGNORECASE)
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

This regex has two named groups both capturing .repo file patterns: 'link' and 'link2'. Having duplicate group names is problematic as only one will be accessible in match results. Additionally, these named groups are never used since only the existence of a match is checked. Consider simplifying to a single pattern without named groups.

Suggested change
token_repo_re = re.compile(r'(?P<link>https?://[^\s"\'<>]+?\.repo\b)|(?P<link2>[^\s"\'<>]+?\.repo\b)', re.IGNORECASE)
token_repo_re = re.compile(r'https?://[^\s"\'<>]+?\.repo\b|[^\s"\'<>]+?\.repo\b', re.IGNORECASE)

Copilot uses AI. Check for mistakes.
if token_repo_re.search(body):
return url

# --- Strategy 2: Debian/Ubuntu Repository Check ---
# Debian repos define a 'Release' or 'InRelease' file at the repository root.
# Matches: href="Release" or href="InRelease"
deb_regex = re.compile(r'href=["\'](?P<link>(?:In)?Release)["\']', re.IGNORECASE)
Comment on lines +73 to +85
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The regex defines a named group 'link' but the named group is not used. The match object from 'search()' is not captured, so the named group serves no purpose. Consider simplifying the regex by removing the named group, as only the existence of a match is checked.

Suggested change
rpm_regex = re.compile(r'href=["\'](?P<link>[^"\']+?\.repo)\b["\']', re.IGNORECASE)
if rpm_regex.search(body):
return url
# Fallback for plain text listings of .repo files
token_repo_re = re.compile(r'(?P<link>https?://[^\s"\'<>]+?\.repo\b)|(?P<link2>[^\s"\'<>]+?\.repo\b)', re.IGNORECASE)
if token_repo_re.search(body):
return url
# --- Strategy 2: Debian/Ubuntu Repository Check ---
# Debian repos define a 'Release' or 'InRelease' file at the repository root.
# Matches: href="Release" or href="InRelease"
deb_regex = re.compile(r'href=["\'](?P<link>(?:In)?Release)["\']', re.IGNORECASE)
rpm_regex = re.compile(r'href=["\'][^"\']+?\.repo\b["\']', re.IGNORECASE)
if rpm_regex.search(body):
return url
# Fallback for plain text listings of .repo files
token_repo_re = re.compile(r'https?://[^\s"\'<>]+?\.repo\b|[^\s"\'<>]+?\.repo\b', re.IGNORECASE)
if token_repo_re.search(body):
return url
# --- Strategy 2: Debian/Ubuntu Repository Check ---
# Debian repos define a 'Release' or 'InRelease' file at the repository root.
# Matches: href="Release" or href="InRelease"
deb_regex = re.compile(r'href=["\'](?:In)?Release["\']', re.IGNORECASE)

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Similar to the RPM regex, the named group 'link' is defined but never used. Consider removing the named group to simplify the code.

Suggested change
deb_regex = re.compile(r'href=["\'](?P<link>(?:In)?Release)["\']', re.IGNORECASE)
deb_regex = re.compile(r'href=["\'](?:In)?Release["\']', re.IGNORECASE)

Copilot uses AI. Check for mistakes.
if deb_regex.search(body):
logging.info(f"Identified Debian-like repository at: {url}")
return url
Comment on lines +71 to +88
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The new validation logic searches for .repo files or Release/InRelease files in the response body (lines 71-88). However, the test mock at jenkins_pipelines/scripts/tests/mock_response.py:22-23 returns a MockResponse with no content/text for successful URLs. This means even the valid test case will fail the new validation checks and return None instead of the expected URL. The mock needs to be updated to include realistic HTML content with .repo file references for successful test cases.

Copilot uses AI. Check for mistakes.

logging.error(f"No .repo, Release, or InRelease found at {url}. Skipping.")
return None

def validate_and_store_results(expected_ids: set [str], custom_repositories: dict[str, dict[str, str]], output_file: str = JSON_OUTPUT_FILE_NAME):
if not custom_repositories:
Expand Down
Loading