PR#297: Reintagrating unretire_packages plugin - fedora-infra/toddlers

fedora-infra / toddlers

#297 Reintagrating unretire_packages plugin

Opened 7 months ago by amedvede. Modified 8 days ago

fedora-infra/ amedvede/toddlers reintegrate_unretirement_plugin into main

unretire_packages: plugin reintagration

Anton Medvedev • 7 months ago

57a649e

tests/plugins/test_unretire_packages.py

file added

+1016

The added file is too large to be shown here, see it at: tests/plugins/test_unretire_packages.py

toddlers/plugins/unretire_packages.py

file added

+573

		`@@ -0,0 +1,573 @@`
		`+ """`
		`+ This is a script to automate unretirement of package automatically, when ticket is created.`
		`+`
		`+ Authors: Anton Medvedev <amedvede@redhat.com>`
		`+`
		`+ """`
		`+`
		`+ import argparse`
		`+ import json`
		`+ import logging`
		`+ import re`
		`+ import sys`
		`+ import tempfile`
		`+ import traceback`
		`+ from typing import Optional`
		`+`
		`+ import arrow`
		`+ from fedora_messaging.api import Message`
		`+ from git import GitCommandError`
		`+ import koji`
		`+ from pagure_messages.issue_schema import IssueNewV1`
		`+ import toml`
		`+`
		`+ from toddlers.base import ToddlerBase`
		`+ from toddlers.exceptions import ValidationError`
		`+ from toddlers.utils import bodhi, bugzilla_system, git, pagure, requests`
		`+`
		`+`
		`+ # Where to look for unretire request tickets`
		`+ PROJECT_NAMESPACE = "releng/fedora-scm-requests"`
		`+ # Keyword that will be searched for in the issue title`
		`+ UNRETIRE_KEYWORD = "unretire"`
		`+ # RPM package prefix, that will be searched in the issue title`
		`+ RPM_PREFIX = "rpms/"`
		`+ # Forbidden keywords for commit message`
		`+ FORBIDDEN_KEYWORDS_FOR_COMMIT_MESSAGE = ["legal", "license"]`
		`+ # Time difference limit not getting Bugzilla url`
		`+ TIME_DIFFERENCE_LIMIT = 56 # 8 weeks in days`
		`+ # Package retirement process url`
		`+ PACKAGE_RETIREMENT_PROCESS_URL = (`
		`+ "https://docs.fedoraproject.org/en-US/package-maintainers"`
		`+ "/Package_Retirement_Process/#claiming"`
		`+ )`
		`+ # Fedora review bugzilla flag`
		`+ FEDORA_REVIEW_FLAG_NAME = "fedora-review"`
		`+ # Koji hub url`
		`+ KOJIHUB_URL = "https://koji.fedoraproject.org/kojihub"`
		`+`
		`+ _log = logging.getLogger(__name__)`
		`+`
		`+`
		`+ class UnretirePackages(ToddlerBase):`
		`+ """`
		`+ Listen for new tickets in https://pagure.io/releng/issues`
zlopez commented 9 days ago `PROJECT_NAMESPACE = "releng/fedora-scm-requests"`, but here it says it's looking for tickets in `https://pagure.io/releng/issues`
		`+ and process then, either by unretiring a package or rejecting the ticket`
		`+ """`
		`+`
		`+ name: str = "unretire_packages"`
		`+`
		`+ amqp_topics: list = ["io.pagure.*.pagure.issue.new"]`
		`+`
		`+ # Path to temporary dir`
		`+ temp_dir: str = ""`
		`+`
		`+ # Requests session`
		`+ requests_session: requests.requests.Session`
		`+`
		`+ # Dist-git base url`
		`+ dist_git_base: Optional[str] = ""`
		`+`
		`+ # Pagure object connected to pagure.io`
		`+ pagure_io: pagure.Pagure`
		`+`
		`+ # Git repo object`
		`+ git_repo: git.GitRepo`
		`+`
		`+ # Koji session object`
		`+ koji_session: koji.ClientSession`
		`+`
		`+ # Bodhi object`
		`+ bodhi: bodhi.Bodhi`
		`+`
		`+ def accepts_topic(self, topic: str) -> bool:`
		`+ """`
		`+ Returns a boolean whether this toddler is interested in messages`
		`+ from this specific topic.`
		`+`
		`+ :arg topic: Topic to check`
		`+`
		`+ :returns: True if topic is accepted, False otherwise`
		`+ """`
		`+ if topic.startswith("io.pagure."):`
		`+ if topic.endswith("pagure.issue.new"):`
		`+ return True`
		`+`
		`+ return False`
		`+`
		`+ def process(`
		`+ self,`
		`+ config: dict,`
		`+ message: Message,`
		`+ ) -> None:`
		`+ """`
		`+ Process a given message.`
		`+`
		`+ :arg config: Toddlers configuration`
		`+ :arg message: Message to process`
		`+ """`
		`+ _log.debug(`
		`+ "Processing message:\n{0}".format(json.dumps(message.body, indent=2))`
		`+ )`
		`+ project_name = message.body["project"]["fullname"]`
		`+`
		`+ if project_name != PROJECT_NAMESPACE:`
		`+ _log.info(`
		`+ "The message doesn't belong to project {0}. Skipping message.".format(`
		`+ PROJECT_NAMESPACE`
		`+ )`
		`+ )`
		`+ return`
		`+`
		`+ issue = message.body["issue"]`
		`+`
		`+ if issue["status"] != "Open":`
		`+ _log.info(`
		`+ "The issue {0} is not open. Skipping message.".format(issue["id"])`
		`+ )`
		`+ return`
		`+`
		`+ issue_title = issue["title"]`
		`+ words_in_issue_title = issue_title.split()`
		`+ if UNRETIRE_KEYWORD != words_in_issue_title[0].lower():`
		`+ _log.info(`
		`+ "The issue doesn't contain keyword '{0}' in the title '{1}'"`
		`+ "".format(UNRETIRE_KEYWORD, issue_title)`
		`+ )`
		`+ return`
		`+`
		`+ _log.debug("Getting temp_folder name from config.")`
		`+ self.temp_dir = config.get("temp_folder", "")`
		`+`
		`+ _log.debug("Creating a request session.")`
		`+ self.requests_session = requests.make_session()`
		`+`
		`+ _log.debug("Getting dist-git url from config.")`
		`+ self.dist_git_base = config.get("dist_git_url")`
		`+`
		`+ _log.debug("Setting up connection to Pagure")`
		`+ self.pagure_io = pagure.set_pagure(config)`
		`+`
		`+ _log.debug("Setting up connection to Bugzilla")`
		`+ bugzilla_system.set_bz(config)`
		`+`
		`+ _log.debug("Setting up session with Koji")`
		`+ self.koji_session = koji.ClientSession(KOJIHUB_URL)`
		`+`
		`+ _log.debug("Setting up bodhi session")`
		`+ self.bodhi = bodhi.set_bodhi(config)`
		`+`
		`+ try:`
		`+ self.process_ticket(issue)`
		`+ except BaseException:`
		`+ self.pagure_io.add_comment_to_issue(`
		`+ issue["id"],`
		`+ namespace=PROJECT_NAMESPACE,`
		`+ comment=(`
		+ "Error happened during processing:\n" "```\n" "{0}\n" "```\n"
		`+ ).format(traceback.format_exc()),`
		`+ )`
		`+`
		`+ def process_ticket(self, issue: dict) -> None:`
		`+ """`
		`+ Process a single ticket`
		`+`
		`+ :arg issue: A dictionary containing the issue`
		`+ """`
		`+ _log.info("Handling pagure releng ticket '{0}'".format(issue["full_url"]))`
		`+ try:`
		`+ # If a ValueError is raised, that means it isn't valid JSON`
		+ issue_body = json.loads(issue["content"].strip("`").strip("\n"))
		`+ except ValueError:`
		`+ _log.info("Invalid JSON in ticket. Closing '{0}'".format(issue["full_url"]))`
		`+ self.pagure_io.close_issue(`
		`+ issue["id"],`
		`+ namespace=PROJECT_NAMESPACE,`
		`+ message="Invalid JSON provided",`
		`+ reason="Invalid",`
		`+ )`
		`+ return`
		`+`
		`+ package_name = issue_body["name"]`
		`+ package_ns = issue_body["type"]`
		`+ maintainer_fas = issue_body["maintainer"]`
		`+`
		`+ package_ns = self._ns_convertor(package_ns)`
		`+`
		`+ package_url = "{0}/{1}/{2}.git".format(`
		`+ self.dist_git_base, package_ns, package_name`
		`+ )`
		`+`
		`+ _log.debug("Verifying that package repository is actually exist.")`
zlopez commented 9 days ago "Verifying that package repository actually exist."
		`+ if not self._is_url_exist(package_url):`
zlopez commented 9 days ago Nothing important, but the name should be `_does_url_exist` to make sense.
		`+ msg = "Package repository doesnt exist. Try to repeat request."`
		`+ _log.info(msg)`
		`+ self.pagure_io.close_issue(`
		`+ issue["id"],`
		`+ namespace=PROJECT_NAMESPACE,`
		`+ message=msg,`
		`+ reason="Invalid",`
		`+ )`
		`+ return`
		`+`
		`+ _log.debug("Creating temporary directory")`
		`+ with tempfile.TemporaryDirectory(dir=self.temp_dir) as tmp_dir:`
		`+ _log.info("Cloning repo into dir with name '{0}'".format(self.temp_dir))`
		`+ try:`
		`+ self.git_repo = git.clone_repo(package_url, tmp_dir)`
		`+ except GitCommandError:`
		`+ message = "Something went wrong during cloning git repository."`
		`+ _log.info(message)`
		`+ self.pagure_io.close_issue(`
		`+ issue["id"],`
		`+ namespace=PROJECT_NAMESPACE,`
		`+ message=message,`
		`+ reason="Invalid",`
		`+ )`
		`+ return`
		`+`
		`+ branches = issue_body["branches"]`
		`+`
		`+ _log.debug("Getting active branches")`
		`+ active_branches = self.bodhi.get_active_branches()`
		`+`
		`+ filtered_branches = [`
		`+ branch for branch in branches if branch in active_branches`
		`+ ]`
		`+`
		`+ final_list_of_branches = []`
		`+ deadpackage_file_path = "dead.package"`
		`+ _log.debug("Verifying that branches are actually exists.")`
		`+ _log.debug(`
		+ "Verifying that branches are actually retired (have a `dead.package` file)."
		`+ )`
		`+ for branch in filtered_branches:`
		`+ if self.git_repo.does_branch_exist(branch):`
		`+ if self.git_repo.does_branch_contains_file(`
		`+ branch, deadpackage_file_path`
		`+ ):`
		`+ final_list_of_branches.append(branch)`
		`+`
		`+ _log.debug("Verifying if package is ready for unretirement.")`
		`+ if not self._is_package_ready_for_unretirement(`
		`+ issue_id=issue["id"],`
		`+ branches=final_list_of_branches,`
		`+ review_bugzilla=issue_body["review_bugzilla"],`
		`+ ):`
		`+ return`
		`+`
		`+ _log.debug("Reverting retire commit")`
		`+ revert_commit_message = "Unretirement request: {0}".format(`
		`+ issue["full_url"]`
		`+ )`
		`+ for branch in final_list_of_branches:`
		`+ self.git_repo.revert_last_commit(`
zlopez commented 8 days ago The method will not work, I needed something similar in retire, which you can see in https://pagure.io/fedora-infra/toddlers/blob/main/f/toddlers/utils/git.py#_159. It took me two weeks to allow the releng bot to push changes to dist-git repository. So you can use what I created.
		`+ message=revert_commit_message, branch=branch`
		`+ )`
		`+`
		`+ _log.debug("Unblocking tags on Koji.")`
		`+ if self._is_need_to_unblock_tags_on_koji(`
zlopez commented 8 days ago Not major problem, but the name of the method should be something like "_check_tags_to_unblock".
		`+ final_list_of_branches, package_name`
		`+ ):`
		`+ _log.debug("Unblocking tags in koji.")`
		`+ for tag in final_list_of_branches:`
		`+ try:`
		`+ self.koji_session.packageListUnblock(`
		`+ taginfo=tag, pkginfo=package_name`
		`+ )`
		`+ except koji.GenericError:`
		+ msg = "Not able to unblock `{0}` tag on koji.".format(tag)
		`+ self.pagure_io.close_issue(`
		`+ issue_id=issue["id"],`
		`+ namespace=PROJECT_NAMESPACE,`
		`+ message=msg,`
		`+ reason="Invalid",`
		`+ )`
		`+ return`
		`+`
		`+ _log.debug("Verifying package is not orphan.")`
		`+ if self.pagure_io.is_project_orphaned(`
		`+ namespace=package_ns, repo=package_name`
		`+ ):`
		`+ if maintainer_fas == "":`
		`+ msg = "Package is ophaned, but maintainer fas is not provided."`
		`+ self.pagure_io.close_issue(`
		`+ issue_id=issue["id"],`
		`+ namespace=PROJECT_NAMESPACE,`
		`+ message=msg,`
		`+ reason="Invalid",`
		`+ )`
		`+ return`
		`+ self.pagure_io.assign_maintainer_to_project(`
		`+ namespace=package_ns,`
		`+ repo=package_name,`
		`+ maintainer_fas=maintainer_fas,`
		`+ )`
		`+`
		`+ _log.info(`
		`+ "Package {0} is assigned to {1}".format(`
		`+ f"{package_ns}/{package_name}", maintainer_fas`
		`+ )`
		`+ )`
		`+ return`
		`+`
		`+ def _is_package_ready_for_unretirement(`
		`+ self, issue_id: int, branches: list, review_bugzilla: str`
		`+ ) -> bool:`
		`+ """`
		`+ Verify that package is ready for unretirement.`
		`+`
		`+ :arg issue_id: An int value of issue ID.`
		`+ :arg branches: A list containing branches that need to be unretired.`
		`+ :arg review_bugzilla: A str contain url on bugzilla review.`
		`+`
		`+ :returns: Bool value whether the package was verified.`
		`+ """`
		`+ try:`
		`+ _log.debug("Verifying the reason of retirement.")`
		`+ self._verify_package_not_retired_for_reason(branches=branches)`
		`+ _log.debug("Verifying the date of retirement.")`
		`+ self._verify_bugzilla_ticket(`
		`+ review_bugzilla=review_bugzilla, branches=branches`
		`+ )`
		`+ except ValidationError as error:`
		`+ self.pagure_io.close_issue(`
		`+ issue_id=issue_id,`
		`+ namespace=PROJECT_NAMESPACE,`
		`+ message=str(error),`
		`+ reason="Invalid",`
		`+ )`
		`+ return False`
		`+ return True`
		`+`
		`+ def _verify_package_not_retired_for_reason(self, branches: list):`
		`+ """`
		`+ Verify that commit message does not contain forbidden keywords.`
		`+`
		`+ Raises:`
		+ `toddler.exceptions.ValidationError`: When retirement reason wasn't verified
		`+ """`
		`+ _log.debug("Verifying that issue message doesn't contain forbidden keywords")`
		`+`
		`+ for branch in branches:`
		`+ last_commit_message = self.git_repo.get_last_commit_message(branch)`
		`+ if any(`
		`+ re.search(forbidden_keyword, str(last_commit_message).lower())`
		`+ for forbidden_keyword in FORBIDDEN_KEYWORDS_FOR_COMMIT_MESSAGE`
		`+ ):`
		`+ raise ValidationError(`
		`+ "Package was retired for a reason: legal of license issue."`
zlopez commented 8 days ago "of" -> "or"
		`+ )`
		`+`
		`+ def _verify_bugzilla_ticket(self, review_bugzilla, branches):`
		`+ """`
		`+ Verify if last commit was made more than 8 weeks ago, need to request a bugzilla ticket.`
		`+ """`
		`+ _log.debug("Verifying that retire commit was made less than 8 weeks ago.")`
		`+`
		`+ is_need_to_verify_bz = False`
		`+`
		`+ for branch in branches:`
		`+ last_commit_date = self.git_repo.get_last_commit_date(branch)`
		`+ if last_commit_date is None:`
		`+ raise ValidationError("Couldn't get a date of the retire commit.")`
		`+ else:`
		`+ last_commit_date = arrow.get(last_commit_date)`
		`+`
		`+ current_time = arrow.utcnow()`
		`+`
		`+ time_diff_in_days = (current_time - last_commit_date).days`
		`+`
		`+ if time_diff_in_days > TIME_DIFFERENCE_LIMIT:`
		`+ is_need_to_verify_bz = True`
		`+`
		`+ if not is_need_to_verify_bz:`
		`+ return`
		`+`
		`+ if review_bugzilla == "":`
		`+ raise ValidationError(`
		`+ "Bugzilla url is missing, please add it and recreate the ticket."`
		`+ )`
		`+`
		`+ bug_id = review_bugzilla`
		`+`
		`+ _log.debug("Getting the bug object from bugzilla.")`
		`+ try:`
		`+ bug = bugzilla_system.get_bug(bug_id)`
		`+ except Exception as error:`
		`+ raise ValidationError(`
		`+ "The Bugzilla bug could not be verified. The following "`
		`+ "error was encountered: {0}".format(str(error))`
		`+ )`
		`+`
		`+ if bug is None:`
		`+ raise ValidationError(`
		`+ "Bugzilla can't get the bug by bug id, fix bugzilla url."`
		`+ )`
		`+`
		`+ if bug.product != "Fedora":`
		`+ raise ValidationError(`
		`+ "The bugzilla bug is for '{0}', "`
		`+ "but request should be for 'Fedora'.".format(bug.product)`
		`+ )`
		`+`
		`+ try:`
		`+ _log.info("Getting {0} flag from bug".format(FEDORA_REVIEW_FLAG_NAME))`
		`+ fedora_review_flag = bug.get_flags(FEDORA_REVIEW_FLAG_NAME)`
		`+ fedora_review_flag_status = fedora_review_flag[0]["status"]`
		`+`
		`+ if fedora_review_flag_status != "+":`
		`+ raise ValidationError(`
		`+ "Flag fedora-review has wrong status, need to be +"`
		`+ )`
		`+ except TypeError:`
		`+ raise ValidationError(`
		`+ "Tag fedora-review is missing on bugzilla, get it and recreate the ticket."`
		`+ )`
		`+`
		`+ def _is_need_to_unblock_tags_on_koji(`
		`+ self, tags_to_unblock: list, repo: str`
		`+ ) -> bool:`
		`+ """`
		`+ Check if at least any of the tags requested to be unblocked are really blocked.`
zlopez commented 8 days ago "at least any" doesn't make sense, should be "at least one"
		`+`
		`+ :arg tags_to_unblock: List of branch names`
		`+ :arg repo: Name of package`
		`+`
		`+ :returns: Bool value whether program need to unblock tags`
		`+ """`
		`+ _log.debug("Verifying that tags are blocked on koji.")`
		`+ try:`
		`+ package_tags = self.koji_session.listTags(package=repo)`
		`+ if not package_tags:`
		`+ raise ValidationError("Package doesn't have tags on koji.")`
		`+ tags_that_suppose_to_be_blocked = []`
		`+`
		`+ for tag in package_tags:`
		`+ prefix = "dist-"`
		`+ if tag["name"].startswith(prefix):`
		`+ tag_name = tag["name"][len(prefix) :] # noqa: E203`
		`+ if tag_name in tags_to_unblock:`
		`+ tags_that_suppose_to_be_blocked.append(tag)`
		`+`
		`+ if len(tags_that_suppose_to_be_blocked) == 0:`
		`+ raise ValidationError(`
		`+ "Request to unblock tags that don't exist on koji."`
		`+ )`
		`+ return any([tag["locked"] for tag in tags_that_suppose_to_be_blocked])`
		`+ except koji.GenericError:`
		`+ raise ValidationError("Package doesn't exist on koji.")`
		`+`
		`+ def _is_url_exist(self, url: str) -> bool:`
		`+ """`
		`+ Check whether url exist.`
		`+`
		`+ :arg url: Url that might exist`
		`+`
		`+ :returns: True if url exist, otherwise False`
		`+ """`
		`+ try:`
		`+ response = self.requests_session.get(url)`
		`+ except ConnectionError:`
		`+ return False`
		`+ return response.status_code == 200`
		`+`
		`+ @staticmethod`
		`+ def _ns_convertor(namespace):`
zlopez commented 9 days ago This could be simply converted to python map. Something like this ns_convertor = { "rpm": "rpms", "test": "tests", "flatpak": "flatpaks", "module": "modules" } Then you can use it like that: ns_convertor[namespace] if namespace in ns_convertor else namespace
		`+ if namespace == "rpm":`
		`+ return "rpms"`
		`+ elif namespace == "test":`
		`+ return "tests"`
		`+ elif namespace == "flatpack":`
zlopez commented 9 days ago The package format is called `flatpak` not `flatpack`
		`+ return "flatpacks"`
		`+ elif namespace == "module":`
		`+ return "modules"`
		`+ else:`
		`+ return namespace`
		`+`
		`+`
		`+ def _get_arguments(args):`
		`+ """Load and parse the CLI arguments.`
		`+`
		`+ :arg args: Script arguments`
		`+`
		`+ :returns: Parsed arguments`
		`+ """`
		`+ parser = argparse.ArgumentParser(`
		`+ description="Processor for Unretire packages, handling tickets from '{}'".format(`
		`+ PROJECT_NAMESPACE`
		`+ )`
		`+ )`
		`+`
		`+ parser.add_argument(`
		`+ "ticket",`
		`+ type=int,`
		`+ help="Number of ticket to process",`
		`+ )`
		`+`
		`+ parser.add_argument(`
		`+ "--config",`
		`+ help="Configuration file",`
		`+ )`
		`+`
		`+ parser.add_argument(`
		`+ "--debug",`
		`+ action="store_const",`
		`+ dest="log_level",`
		`+ const=logging.DEBUG,`
		`+ default=logging.INFO,`
		`+ help="Enable debugging output",`
		`+ )`
		`+ return parser.parse_args(args)`
		`+`
		`+`
		`+ def _setup_logging(log_level: int) -> None:`
		`+ """`
		`+ Set up the logging level.`
		`+`
		`+ :arg log_level: Log level to set`
		`+ """`
		`+ handlers = []`
		`+`
		`+ _log.setLevel(log_level)`
		`+ # We want all messages logged at level INFO or lower to be printed to stdout`
		`+ info_handler = logging.StreamHandler(stream=sys.stdout)`
		`+ handlers.append(info_handler)`
		`+`
		`+ if log_level == logging.INFO:`
		`+ # In normal operation, don't decorate messages`
		`+ for handler in handlers:`
		`+ handler.setFormatter(logging.Formatter("%(message)s"))`
		`+`
		`+ logging.basicConfig(level=log_level, handlers=handlers)`
		`+`
		`+`
		`+ def main(args):`
		`+ """Main function"""`
		`+ args = _get_arguments(args)`
		`+ _setup_logging(log_level=args.log_level)`
		`+ _log.info("hello i'm starting work")`
		`+`
		`+ config = toml.load(args.config)`
		`+`
		`+ ticket = args.ticket`
		`+`
		`+ pagure_io = pagure.set_pagure(config)`
		`+ issue = pagure_io.get_issue(ticket, PROJECT_NAMESPACE)`
		`+`
		`+ # Convert issue to message`
		`+ body = {"project": {"fullname": PROJECT_NAMESPACE}, "issue": issue}`
		`+ message = IssueNewV1(body=body)`
		`+ _log.debug("Message prepared: {}".format(message.body))`
		`+`
		`+ UnretirePackages().process(`
		`+ config=config,`
		`+ message=message,`
		`+ )`
		`+`
		`+`
		`+ if __name__ == "__main__": # pragma: no cover`
		`+ try:`
		`+ main(sys.argv[1:])`
		`+ except KeyboardInterrupt:`
		`+ pass`

amedvede commented 7 months ago

Signed-off-by: Anton Medvedev amedvede@redhat.com

zuul commented 7 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci
https://fedora.softwarefactory-project.io/zuul/buildset/1ed38ee8cd62400ab47bb6621c4420c0

fi-tox-mypy : FAILURE in 6m 26s
fi-tox-format : FAILURE in 6m 12s
fi-tox-lint : FAILURE in 6m 04s
fi-tox-python39 : FAILURE in 7m 31s
fi-tox-python310 : FAILURE in 7m 24s
fi-tox-python311 : FAILURE in 7m 16s
fi-tox-python312 : FAILURE in 7m 24s

1 new commit added

unretire_packages: the whole plugin was fixed

7 months ago

zuul commented 7 months ago

fi-tox-mypy : SUCCESS in 6m 00s
fi-tox-format : FAILURE in 6m 04s
fi-tox-lint : FAILURE in 5m 58s
fi-tox-python39 : FAILURE in 7m 25s
fi-tox-python310 : FAILURE in 7m 44s
fi-tox-python311 : FAILURE in 6m 48s
fi-tox-python312 : FAILURE in 7m 02s

1 new commit added

unretire_packages: added tests but not finished, also small correction in plugin

7 months ago

zuul commented 7 months ago

fi-tox-mypy : SUCCESS in 6m 12s
fi-tox-format : FAILURE in 5m 38s
fi-tox-lint : FAILURE in 5m 53s
fi-tox-python39 : FAILURE in 7m 05s
fi-tox-python310 : FAILURE in 6m 55s
fi-tox-python311 : FAILURE in 6m 52s
fi-tox-python312 : FAILURE in 7m 14s

1 new commit added

unretire_packages: even more test

7 months ago

zuul commented 7 months ago

fi-tox-mypy : SUCCESS in 5m 56s
fi-tox-format : FAILURE in 5m 41s
fi-tox-lint : FAILURE in 5m 53s
fi-tox-python39 : FAILURE in 7m 03s
fi-tox-python310 : FAILURE in 6m 31s
fi-tox-python311 : FAILURE in 7m 03s
fi-tox-python312 : FAILURE in 7m 07s

1 new commit added

unretire_packages: code covered on 99% by tests

7 months ago

zuul commented 7 months ago

fi-tox-mypy : SUCCESS in 9m 40s
fi-tox-format : FAILURE in 15m 37s
fi-tox-lint : FAILURE in 7m 17s
fi-tox-python39 : FAILURE in 10m 44s
fi-tox-python310 : FAILURE in 16m 30s
fi-tox-python311 : FAILURE in 8m 29s
fi-tox-python312 : FAILURE in 11m 11s

1 new commit added

unretire_packages: format changes

7 months ago

zuul commented 7 months ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/2f777aa45a5148149025ab85fed17361

fi-tox-mypy : SUCCESS in 5m 42s
fi-tox-format : SUCCESS in 5m 16s
fi-tox-lint : SUCCESS in 6m 04s
fi-tox-python39 : SUCCESS in 7m 18s
fi-tox-python310 : SUCCESS in 7m 22s
fi-tox-python311 : SUCCESS in 7m 34s
fi-tox-python312 : SUCCESS in 7m 30s

rebased onto 3557ca9

7 months ago

zuul commented 7 months ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/b3971f38179344998c34842cae065a8b

fi-tox-mypy : SUCCESS in 5m 51s
fi-tox-format : SUCCESS in 5m 55s
fi-tox-lint : SUCCESS in 5m 51s
fi-tox-python39 : SUCCESS in 7m 25s
fi-tox-python310 : SUCCESS in 6m 52s
fi-tox-python311 : SUCCESS in 6m 37s
fi-tox-python312 : SUCCESS in 6m 40s

zlopez commented on line 31 of toddlers/plugins/unretire_packages.py 7 months ago

Was this regex tested?

zlopez commented on line 30 of toddlers/plugins/unretire_packages.py 7 months ago

I would probably move all constants as configurable values. In case of any future change it will be just configuration change

zlopez commented on line 114 of toddlers/plugins/unretire_packages.py 7 months ago

You should use a pagure messaging schema if available.

zlopez commented on line 141 of toddlers/plugins/unretire_packages.py 7 months ago

Move it as debug message instead of removing.

zlopez commented on line 142 of toddlers/plugins/unretire_packages.py 7 months ago

Why do you need a configurable temp_folder? Just use TemporaryDirectory from python. https://docs.python.org/3/library/tempfile.html#tempfile.TemporaryDirectory

EDIT: Nevermind, I remembered that you need it configurable for OpenShift and you are already using TemporaryDirectory for that :-)

Edited 7 months ago by zlopez

zlopez commented on line 198 of toddlers/plugins/unretire_packages.py 7 months ago

This shouldn't be a TODO in the code.

zlopez commented on line 194 of toddlers/plugins/unretire_packages.py 7 months ago

This shouldn't be a TODO in the code.

zlopez commented on line 204 of toddlers/plugins/unretire_packages.py 7 months ago

"Verifying that package repository actually exist."

zlopez commented on line 206 of toddlers/plugins/unretire_packages.py 7 months ago

"Package repository doesn't exist, please check package name and reopen a ticket."

I don't think that this toddler will even catch message that will be send when re-opening closed ticket.

Edited 7 months ago by zlopez

zlopez commented on line 216 of toddlers/plugins/unretire_packages.py 7 months ago

Plenty of the messages could be moved to debug, as you don't need to see them till you are debugging some issue.

zlopez commented on line 252 of toddlers/plugins/unretire_packages.py 7 months ago

Why don't you do both checks in one cycle? You don't need to go through the branches twice.

Edited 7 months ago by zlopez

zlopez commented on line 287 of toddlers/plugins/unretire_packages.py 7 months ago

This print should be removed.

zlopez commented on line 303 of toddlers/plugins/unretire_packages.py 7 months ago

This should probably say something like "Package <name> is assigned to <maintainer_fas>". That is a good info message to have.

zlopez commented on line 320 of toddlers/plugins/unretire_packages.py 7 months ago

No need for a new method as this code is used only once.

zlopez commented on line 322 of toddlers/plugins/unretire_packages.py 7 months ago

No need for a new method as this code is used only once.

zlopez commented on line 368 of toddlers/plugins/unretire_packages.py 7 months ago

Maybe arrow would be a better suited library as I don't understand why you need to convert the date to float.

zlopez commented on line 381 of toddlers/plugins/unretire_packages.py 7 months ago

No need for a new method as this code is used only once.

zlopez commented on line 435 of toddlers/plugins/unretire_packages.py 7 months ago

This makes sense as separate method as it is used as a boolean validation.

zlopez commented on line 462 of toddlers/plugins/unretire_packages.py 7 months ago

"Request to unblock tags that don't exist on koji."

zlopez commented on line 468 of toddlers/plugins/unretire_packages.py 7 months ago

No need for a new method as this code is used only once.

zlopez commented on line 483 of toddlers/plugins/unretire_packages.py 7 months ago

This print should be removed.

zlopez commented on line 501 of toddlers/plugins/unretire_packages.py 7 months ago

You should use the session that is already available in toddlers utils.requests.

zlopez commented on line 570 of toddlers/plugins/unretire_packages.py 7 months ago

Why this? Everything is already in config.

zlopez commented on line 571 of toddlers/plugins/unretire_packages.py 7 months ago

Why this? Everything is already in config.

1 new commit added

unretire_packages: refactoring by review

7 months ago

zuul commented 7 months ago

fi-tox-mypy : SUCCESS in 5m 20s
fi-tox-format : SUCCESS in 6m 04s
fi-tox-lint : SUCCESS in 6m 48s
fi-tox-python39 : FAILURE in 7m 22s
fi-tox-python310 : FAILURE in 7m 17s
fi-tox-python311 : FAILURE in 7m 22s
fi-tox-python312 : SUCCESS in 7m 56s

1 new commit added

update

7 months ago

zuul commented 7 months ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/e8833f47286c49d38162904c0f2e81d9

fi-tox-mypy : SUCCESS in 6m 42s
fi-tox-format : SUCCESS in 6m 05s
fi-tox-lint : SUCCESS in 6m 20s
fi-tox-python39 : SUCCESS in 6m 52s
fi-tox-python310 : SUCCESS in 7m 22s
fi-tox-python311 : SUCCESS in 6m 42s
fi-tox-python312 : SUCCESS in 7m 29s

rebased onto 3a638dc

7 months ago

zuul commented 7 months ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/6ec1013fce924946be357257ec5f8fb6

fi-tox-mypy : SUCCESS in 6m 30s
fi-tox-format : SUCCESS in 5m 55s
fi-tox-lint : SUCCESS in 6m 45s
fi-tox-python39 : SUCCESS in 6m 55s
fi-tox-python310 : SUCCESS in 7m 33s
fi-tox-python311 : SUCCESS in 6m 52s
fi-tox-python312 : SUCCESS in 7m 32s

zlopez commented on line 30 of toddlers/plugins/unretire_packages.py 7 months ago

As I said previously this would be better to make configurable through config file.

zlopez commented on line 342 of toddlers/plugins/unretire_packages.py 7 months ago

This doesn't need to be separate method.

zlopez commented on line 361 of toddlers/plugins/unretire_packages.py 7 months ago

This doesn't need to be separate method.

zlopez commented on line 460 of toddlers/plugins/unretire_packages.py 7 months ago

This doesn't need to be separate method.

zlopez commented 7 months ago

Looks better and we can merge it as is. I just added few improvements that could be implemented.

Did you tried to test it locally?

amedvede commented 7 months ago

I'm testing it locally now, already found one issue, that plugin use namespace that is passed by fedpkg, and in fedpkg type(namespace) is rpm, but plugin need to userpms so I need to write convertor

1 new commit added

unretire_packages: added namespace convertor

7 months ago

zuul commented 7 months ago

fi-tox-mypy : SUCCESS in 6m 13s
fi-tox-format : FAILURE in 6m 12s
fi-tox-lint : FAILURE in 5m 52s
fi-tox-python39 : SUCCESS in 7m 13s
fi-tox-python310 : SUCCESS in 7m 02s
fi-tox-python311 : SUCCESS in 6m 47s
fi-tox-python312 : SUCCESS in 7m 02s

rebased onto 3a638dc

7 months ago

amedvede commented 7 months ago

added ns convertor and fixed format a little

zuul commented 7 months ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/dd67576f2234424191f4879bd8986b07

fi-tox-mypy : SUCCESS in 6m 23s
fi-tox-format : SUCCESS in 5m 17s
fi-tox-lint : SUCCESS in 6m 20s
fi-tox-python39 : SUCCESS in 6m 27s
fi-tox-python310 : SUCCESS in 6m 46s
fi-tox-python311 : SUCCESS in 7m 08s
fi-tox-python312 : SUCCESS in 6m 56s

zlopez commented on line 55 of toddlers/plugins/unretire_packages.py 9 days ago

PROJECT_NAMESPACE = "releng/fedora-scm-requests", but here it says it's looking for tickets in https://pagure.io/releng/issues

zlopez commented on line 202 of toddlers/plugins/unretire_packages.py 9 days ago

"Verifying that package repository actually exist."

zlopez commented on line 203 of toddlers/plugins/unretire_packages.py 9 days ago

Nothing important, but the name should be _does_url_exist to make sense.

zlopez commented on line 477 of toddlers/plugins/unretire_packages.py 9 days ago

This could be simply converted to python map. Something like this

ns_convertor = {
  "rpm": "rpms",
  "test": "tests",
  "flatpak": "flatpaks",
  "module": "modules"
}

Then you can use it like that:

ns_convertor[namespace] if namespace in ns_convertor else namespace

zlopez commented on line 482 of toddlers/plugins/unretire_packages.py 9 days ago

The package format is called flatpak not flatpack

zlopez commented on line 265 of toddlers/plugins/unretire_packages.py 8 days ago

The method will not work, I needed something similar in retire, which you can see in https://pagure.io/fedora-infra/toddlers/blob/main/f/toddlers/utils/git.py#_159. It took me two weeks to allow the releng bot to push changes to dist-git repository. So you can use what I created.

zlopez commented on line 270 of toddlers/plugins/unretire_packages.py 8 days ago

Not major problem, but the name of the method should be something like "_check_tags_to_unblock".

zlopez commented on line 433 of toddlers/plugins/unretire_packages.py 8 days ago

"at least any" doesn't make sense, should be "at least one"

zlopez commented on line 360 of toddlers/plugins/unretire_packages.py 8 days ago

"of" -> "or"

zlopez commented 8 days ago

Found few more things and as I was working on retirement of branch in scm_request_processor I found out that the unretire git operation will not work as it is currently.