From eb943197a1a585070ad6c2f35fc008182ce15b15 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jan 05 2021 10:25:31 +0000 Subject: [PATCH 1/5] Use relative imports to avoid ambiguity Signed-off-by: Nils Philippsen --- diff --git a/fedscm_admin/__init__.py b/fedscm_admin/__init__.py index 92b143c..bba15a0 100644 --- a/fedscm_admin/__init__.py +++ b/fedscm_admin/__init__.py @@ -22,7 +22,7 @@ from copy import deepcopy import pkg_resources -import fedscm_admin.config +from . import config try: VERSION = pkg_resources.get_distribution('fedscm-admin').version @@ -108,7 +108,7 @@ for branch, slas in deepcopy(STANDARD_BRANCH_SLAS).items(): for sla_eol in slas.values()]): STANDARD_BRANCH_SLAS.pop(branch) -CONFIG = fedscm_admin.config.get_config() +CONFIG = config.get_config() # This is defined here to avoid circular imports @@ -121,8 +121,8 @@ def is_epel(branch): return bool(re.match(r'^(?:el|epel)\d+$', branch)) -from fedscm_admin.fas import FASClient # noqa: E402 +from .fas import FASClient # noqa: E402 FAS_CLIENT = FASClient() -from fedscm_admin.bugzilla import BugzillaClient # noqa: E402 +from .bugzilla import BugzillaClient # noqa: E402 BUGZILLA_CLIENT = BugzillaClient() diff --git a/fedscm_admin/bugzilla.py b/fedscm_admin/bugzilla.py index b8000c9..8336e8a 100644 --- a/fedscm_admin/bugzilla.py +++ b/fedscm_admin/bugzilla.py @@ -28,8 +28,8 @@ try: except ImportError: from bugzilla import BugzillaError -from fedscm_admin.exceptions import ValidationError -from fedscm_admin import FAS_CLIENT, is_epel +from . import FAS_CLIENT, is_epel +from .exceptions import ValidationError class BugzillaClient(object): diff --git a/fedscm_admin/fedscm_admin.py b/fedscm_admin/fedscm_admin.py index 44edd96..b5df255 100644 --- a/fedscm_admin/fedscm_admin.py +++ b/fedscm_admin/fedscm_admin.py @@ -19,12 +19,11 @@ from __future__ import absolute_import import click -from fedscm_admin.utils import ( - process_all_tickets, list_all_tickets, process_ticket, - login_to_bugzilla_with_user_input, login_to_fas_with_user_input) -import fedscm_admin.pagure -import fedscm_admin.config -from fedscm_admin import CONFIG +from . import CONFIG, config, pagure +from .utils import list_all_tickets +from .utils import login_to_bugzilla_with_user_input +from .utils import login_to_fas_with_user_input +from .utils import process_all_tickets, process_ticket ACTION_CHOICES = ['list', 'process', 'processall'] @@ -60,7 +59,7 @@ def cli(ticket_id, action, auto_approve, force): # Test that all the config items are set in config.ini for config_item in ['pagure_api_token', 'pagure_ticket_api_token', 'pdc_api_token']: - fedscm_admin.config.get_config_item(CONFIG, config_item) + config.get_config_item(CONFIG, config_item) if action == 'processall': # We need to authenticate to FAS to see if a package reviewer is a @@ -73,7 +72,7 @@ def cli(ticket_id, action, auto_approve, force): elif action == 'process': if ticket_id is None: raise click.ClickException('You must specify a ticket ID') - issue = fedscm_admin.pagure.get_issue(ticket_id) + issue = pagure.get_issue(ticket_id) if issue is None or issue.get('status') != 'Open': raise click.ClickException( 'The ticket does not exist or is closed') diff --git a/fedscm_admin/pagure.py b/fedscm_admin/pagure.py index 1df5735..53e49f4 100644 --- a/fedscm_admin/pagure.py +++ b/fedscm_admin/pagure.py @@ -20,10 +20,9 @@ from six.moves.urllib.parse import urlencode import click -from fedscm_admin import CONFIG -from fedscm_admin.config import get_config_item -from fedscm_admin.request_utils import ( - get_auth_header, requests_wrapper, get_request_json) +from . import CONFIG +from .config import get_config_item +from .request_utils import get_auth_header, get_request_json, requests_wrapper def get_pagure_auth_header(token_type='ticket'): diff --git a/fedscm_admin/pdc.py b/fedscm_admin/pdc.py index 6d89b99..edc8fda 100644 --- a/fedscm_admin/pdc.py +++ b/fedscm_admin/pdc.py @@ -20,11 +20,10 @@ from six.moves.urllib.parse import urlencode import click -from fedscm_admin import CONFIG -from fedscm_admin.config import get_config_item -from fedscm_admin.request_utils import ( - requests_wrapper, get_request_json, get_auth_header) -from fedscm_admin.exceptions import ValidationError +from . import CONFIG +from .config import get_config_item +from .exceptions import ValidationError +from .request_utils import get_auth_header, get_request_json, requests_wrapper def get_pdc_auth_header(): diff --git a/fedscm_admin/request_utils.py b/fedscm_admin/request_utils.py index 4ea9662..9dc1bcc 100644 --- a/fedscm_admin/request_utils.py +++ b/fedscm_admin/request_utils.py @@ -24,8 +24,8 @@ from requests.adapters import HTTPAdapter from requests.exceptions import ConnectTimeout, ConnectionError import click -from fedscm_admin.config import get_config_item -from fedscm_admin import CONFIG, VERSION +from . import CONFIG, VERSION +from .config import get_config_item def retry_session(): diff --git a/fedscm_admin/utils.py b/fedscm_admin/utils.py index 9849be0..78582b6 100644 --- a/fedscm_admin/utils.py +++ b/fedscm_admin/utils.py @@ -25,15 +25,11 @@ import click from six import string_types from six.moves import xmlrpc_client -from fedscm_admin.config import get_config_item -from fedscm_admin import ( - CONFIG, MONITOR_CHOICES, BUGZILLA_CLIENT, FAS_CLIENT, STANDARD_BRANCH_SLAS, - INVALID_EPEL_ERROR, is_epel) -import fedscm_admin.pdc -import fedscm_admin.pagure -import fedscm_admin.git -from fedscm_admin.request_utils import requests_wrapper, get_request_json -from fedscm_admin.exceptions import ValidationError +from . import CONFIG, BUGZILLA_CLIENT, FAS_CLIENT, INVALID_EPEL_ERROR +from . import MONITOR_CHOICES, STANDARD_BRANCH_SLAS, git, is_epel, pagure, pdc +from .config import get_config_item +from .exceptions import ValidationError +from .request_utils import get_request_json, requests_wrapper def login_to_bugzilla_with_user_input(): @@ -108,7 +104,7 @@ def verify_slas(branch, sla_dict): raise ValidationError( 'The EOL date "{0}" is in an invalid format'.format(eol)) - sla_obj = fedscm_admin.pdc.get_sla(sla) + sla_obj = pdc.get_sla(sla) if not sla_obj: raise ValidationError('The SL "{0}" is not in PDC'.format(sla)) @@ -179,7 +175,7 @@ def list_all_tickets(): :return: None """ # Sort the issues so that the oldest get shown first - issues = fedscm_admin.pagure.get_issues() + issues = pagure.get_issues() for issue in issues: issue_id = issue['id'] issue_title = issue['title'].strip() @@ -197,7 +193,7 @@ def process_all_tickets(auto_approve=False): :return: None """ # Sort the issues so that the oldest get processed first - issues = fedscm_admin.pagure.get_issues() + issues = pagure.get_issues() for issue in issues: process_ticket(issue, auto_approve=auto_approve) @@ -353,17 +349,17 @@ def prompt_for_new_repo(issue_json, issue_body_json, force=False, click.echo('- Checking if user {0} has an account in dist-git.'.format( issue_owner)) pagure_url = get_config_item(CONFIG, 'pagure_dist_git_url') - if not fedscm_admin.pagure.user_exists(issue_owner): + if not pagure.user_exists(issue_owner): sync_comment = ('@{0} needs to login to {1} to sync accounts ' 'before we can proceed.'.format(issue_owner, pagure_url)) question = '{0} Post this comment to the ticket?'.format(sync_comment) if click.confirm(question): - fedscm_admin.pagure.add_comment_to_issue(issue_id, sync_comment) + pagure.add_comment_to_issue(issue_id, sync_comment) return click.echo('- Checking if {0}/{1} already exists in dist-git.'.format( namespace, repo)) - project = fedscm_admin.pagure.get_project(namespace, repo) + project = pagure.get_project(namespace, repo) if project: prompt_to_close_bad_ticket( issue_json, 'The Pagure project already exists') @@ -371,15 +367,15 @@ def prompt_for_new_repo(issue_json, issue_body_json, force=False, description = issue_body_json.get('description', '').strip() upstreamurl = issue_body_json.get('upstreamurl', '').strip() - component_type = fedscm_admin.pdc.component_type_to_singular(namespace) + component_type = pdc.component_type_to_singular(namespace) master_branch = None if branch_name != 'master': click.echo('- Checking if master already exists in PDC.') - master_branch = fedscm_admin.pdc.get_branch( + master_branch = pdc.get_branch( repo, 'master', component_type) click.echo('- Checking if {0} already exists in PDC.'.format(branch_name)) - branch = fedscm_admin.pdc.get_branch(repo, branch_name, component_type) + branch = pdc.get_branch(repo, branch_name, component_type) if master_branch or branch: prompt_to_close_bad_ticket( @@ -387,7 +383,7 @@ def prompt_for_new_repo(issue_json, issue_body_json, force=False, return issue_title = issue_json['title'].strip() - issue_ui_url = fedscm_admin.pagure.get_pagure_issue_url(issue_id) + issue_ui_url = pagure.get_pagure_issue_url(issue_id) bz_bug_url = '' if bug_id: bz_bug_url = BUGZILLA_CLIENT.get_bug_url(bug_id) @@ -428,27 +424,27 @@ def prompt_for_new_repo(issue_json, issue_body_json, force=False, # If the global component already exists, this will not create another # Skip for tests namespace if namespace != 'tests': - fedscm_admin.pdc.new_global_component(repo, dist_git_url) + pdc.new_global_component(repo, dist_git_url) # Pagure uses plural names for namespaces, but PDC does not use the # plural version for branch types - branch_type = fedscm_admin.pdc.component_type_to_singular(namespace) + branch_type = pdc.component_type_to_singular(namespace) # If the branch requested isn't master, still create a master branch # in PDC anyways. # Skip pdc magic for tests namespace if namespace != 'tests': if branch_name != 'master': - fedscm_admin.pdc.new_branch(repo, 'master', branch_type) - for sla, eol in fedscm_admin.STANDARD_BRANCH_SLAS['master'].items(): - fedscm_admin.pdc.new_sla_to_branch( + pdc.new_branch(repo, 'master', branch_type) + for sla, eol in STANDARD_BRANCH_SLAS['master'].items(): + pdc.new_sla_to_branch( sla, eol, repo, 'master', branch_type) - fedscm_admin.pdc.new_branch(repo, branch_name, branch_type) + pdc.new_branch(repo, branch_name, branch_type) for sla, eol in issue_body_json['sls'].items(): - fedscm_admin.pdc.new_sla_to_branch( + pdc.new_sla_to_branch( sla, eol, repo, branch_name, branch_type) # Create the Pagure repo - fedscm_admin.pagure.new_project( + pagure.new_project( namespace, repo, description, upstreamurl, initial_commit=initial_commit) # If the branch requested isn't master, create that branch in git. The @@ -456,9 +452,9 @@ def prompt_for_new_repo(issue_json, issue_body_json, force=False, if branch_name != 'master': new_git_branch(namespace, repo, branch_name, use_master=True) - fedscm_admin.pagure.set_monitoring_status( + pagure.set_monitoring_status( namespace, repo, issue_body_json['monitor'].strip()) - fedscm_admin.pagure.change_project_main_admin( + pagure.change_project_main_admin( namespace, repo, issue_owner) if branch_name == 'master': @@ -473,7 +469,7 @@ def prompt_for_new_repo(issue_json, issue_body_json, force=False, elif action == 'deny': comment_body = click.prompt( 'Please enter a comment explaining the denial') - fedscm_admin.pagure.close_issue(issue_id, comment_body, 'Denied') + pagure.close_issue(issue_id, comment_body, 'Denied') if bug_id: BUGZILLA_CLIENT.comment(bug_id, comment_body) else: @@ -514,7 +510,7 @@ def prompt_for_new_branch(issue_json, issue_body_json, force=False, auto_approve bug_id = str(issue_body_json.get('bug_id', '')).strip() create_git_branch = issue_body_json.get('create_git_branch', True) - project = fedscm_admin.pagure.get_project(namespace, repo) + project = pagure.get_project(namespace, repo) if not project: prompt_to_close_bad_ticket( issue_json, 'The Pagure repo does not exist') @@ -530,9 +526,9 @@ def prompt_for_new_branch(issue_json, issue_body_json, force=False, auto_approve return # Pagure uses plural names for namespaces, but PDC does not use the # plural version for branch types - branch_type = fedscm_admin.pdc.component_type_to_singular(namespace) + branch_type = pdc.component_type_to_singular(namespace) click.echo('- Checking if {0} already exists in PDC.'.format(branch_name)) - pdc_branch = fedscm_admin.pdc.get_branch(repo, branch_name, branch_type) + pdc_branch = pdc.get_branch(repo, branch_name, branch_type) if pdc_branch: ticket_text = \ "The branch in PDC already exists, you can now create it yourself as follows:\n" \ @@ -549,7 +545,7 @@ def prompt_for_new_branch(issue_json, issue_body_json, force=False, auto_approve issue_id = issue_json['id'] issue_title = issue_json['title'].strip() issue_owner = issue_json['user']['name'] - issue_ui_url = fedscm_admin.pagure.get_pagure_issue_url(issue_id) + issue_ui_url = pagure.get_pagure_issue_url(issue_id) # Check if the branch requestor is one of the maintainers or part of the groups click.echo('- Checking if {0} is one of the maintainers of the package'.format(issue_owner)) @@ -605,14 +601,14 @@ def prompt_for_new_branch(issue_json, issue_body_json, force=False, auto_approve pagure_url.rstrip('/'), namespace, repo) # If the global component already exists, this will not try to create # it - fedscm_admin.pdc.new_global_component(repo, dist_git_url) + pdc.new_global_component(repo, dist_git_url) # Pagure uses plural names for namespaces, but PDC does not use the # plural version for branch types - branch_type = fedscm_admin.pdc.component_type_to_singular(namespace) + branch_type = pdc.component_type_to_singular(namespace) - fedscm_admin.pdc.new_branch(repo, branch_name, branch_type) + pdc.new_branch(repo, branch_name, branch_type) for sla, eol in issue_body_json['sls'].items(): - fedscm_admin.pdc.new_sla_to_branch( + pdc.new_sla_to_branch( sla, eol, repo, branch_name, branch_type) if create_git_branch: @@ -630,7 +626,7 @@ def prompt_for_new_branch(issue_json, issue_body_json, force=False, auto_approve elif action == 'deny': comment_body = click.prompt( 'Please enter a comment explaining the denial') - fedscm_admin.pagure.close_issue(issue_id, comment_body, 'Denied') + pagure.close_issue(issue_id, comment_body, 'Denied') if bug_id: BUGZILLA_CLIENT.comment(bug_id, comment_body) else: @@ -655,7 +651,7 @@ def comment_and_close_ticket(issue_id, bug_id, comment=None, :return: None """ if comment is not None: - fedscm_admin.pagure.add_comment_to_issue(issue_id, comment) + pagure.add_comment_to_issue(issue_id, comment) if bug_id: BUGZILLA_CLIENT.comment(bug_id, comment) click.echo('The following comment was added to the issue "{0}"' @@ -665,7 +661,7 @@ def comment_and_close_ticket(issue_id, bug_id, comment=None, if click.confirm('Would you like to add another comment?'): custom_comment = click.prompt('Please enter a comment') - fedscm_admin.pagure.close_issue(issue_id, custom_comment, close_status) + pagure.close_issue(issue_id, custom_comment, close_status) if custom_comment and bug_id: BUGZILLA_CLIENT.comment(bug_id, custom_comment) @@ -699,7 +695,7 @@ def prompt_to_close_bad_ticket(issue_json, error='Invalid ticket body'): if click.confirm('Would you like to replace the default comment of ' '"{0}"?'.format(error)): comment_body = click.prompt('Please enter a comment') - fedscm_admin.pagure.close_issue(issue_id, comment_body, 'Invalid') + pagure.close_issue(issue_id, comment_body, 'Invalid') if bug_id: BUGZILLA_CLIENT.comment(bug_id, comment_body) @@ -778,10 +774,10 @@ def assert_git_repo_initialized_remotely(namespace, repo): :return: None or ValidationError """ click.echo('- Verifying that the git repo is initialized') - git_url = fedscm_admin.pagure.get_project_git_url( + git_url = pagure.get_project_git_url( namespace, repo, url_type='git', username=FAS_CLIENT.client.username) - git_obj = fedscm_admin.git.GitRepo(git_url) + git_obj = git.GitRepo(git_url) if not git_obj.initialized_remotely: raise ValidationError('The git repository is not initialized. The git ' 'branch can\'t be created.') @@ -800,21 +796,21 @@ def new_git_branch(namespace, repo, branch, use_master=False): :return: None or ValidationError """ if use_master is True: - fedscm_admin.pagure.new_branch( + pagure.new_branch( namespace, repo, branch, from_branch='master') else: # Even though the branches are created using pagure api which dont # require ssh, but the code supports adding package.cfg file. # This should be pushed using ssh. - git_url = fedscm_admin.pagure.get_project_git_url( + git_url = pagure.get_project_git_url( namespace, repo, url_type='ssh', username=FAS_CLIENT.client.username) - git_obj = fedscm_admin.git.GitRepo(git_url) + git_obj = git.GitRepo(git_url) git_obj.clone_repo() if not git_obj.initialized: raise ValidationError('The git repository is not initialized. A ' 'git branch can\'t be created.') - fedscm_admin.pagure.new_branch( + pagure.new_branch( namespace, repo, branch, from_commit=git_obj.first_commit) From b19f90fe5823e35e0a342cf79cbabb53ae5338a3 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jan 05 2021 12:15:24 +0000 Subject: [PATCH 2/5] Don't use real configuration in tests Signed-off-by: Nils Philippsen --- diff --git a/fedscm_admin/config.py b/fedscm_admin/config.py index d848fbb..82c2adf 100644 --- a/fedscm_admin/config.py +++ b/fedscm_admin/config.py @@ -36,7 +36,7 @@ def get_config(): if os.environ.get('FEDSCM_ADMIN_TEST_CONFIG', 'false').lower() == 'true': test_config_path = os.path.abspath(os.path.join( os.path.dirname(__file__), '../tests/test_config.ini')) - paths.append(test_config_path) + paths = [test_config_path] custom_config_path = os.environ.get('FEDSCM_ADMIN_CONFIG') if custom_config_path: From 7cbc04e6e5350194c3ebf59ce62006fe93e285df Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jan 05 2021 12:15:50 +0000 Subject: [PATCH 3/5] Use testing configuration in all tests Signed-off-by: Nils Philippsen --- diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..9326d1b --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,17 @@ +import os + +import pytest + + +@pytest.fixture +def saved_environ(): + saved_environ = dict(os.environ) + yield os.environ + os.environ.clear() + os.environ.update(saved_environ) + + +@pytest.fixture(autouse=True) +def test_config(saved_environ): + saved_environ["FEDSCM_ADMIN_TEST_CONFIG"] = "true" + yield From 386ed9fc02eb95618ea3a308feff9ccd820dbf45 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jan 05 2021 12:39:51 +0000 Subject: [PATCH 4/5] Fix commented out test Ignore the order of lines rather than not verifying the output. Signed-off-by: Nils Philippsen --- diff --git a/tests/test_admin.py b/tests/test_admin.py index 9af2b86..ea1a7c0 100644 --- a/tests/test_admin.py +++ b/tests/test_admin.py @@ -109,11 +109,12 @@ class FedScmAdmin(TestCase): runner = CliRunner() result = runner.invoke(fedscm_admin_cli, ['list']) - expected_rv = ('#1: New Repo for "rpms/nethack" (opened by akhairna)\n' - '#2: New Branch "abc" for "rpms/nethack" (opened by ' - 'akhairna)\n') + expected_lines = { + '#1: New Repo for "rpms/nethack" (opened by akhairna)', + '#2: New Branch "abc" for "rpms/nethack" (opened by akhairna)', + } assert result.exit_code == 0 - #assert result.output == expected_rv + assert {line for line in result.output.split("\n") if line} == expected_lines @patch('fedscm_admin.utils.verify_slas', return_value=None) @patch('fedscm_admin.request_utils.retry_session') From 753692d06ecb8c42caab51fc21784f92f4a61850 Mon Sep 17 00:00:00 2001 From: Nils Philippsen Date: Jan 05 2021 12:40:01 +0000 Subject: [PATCH 5/5] Wrap too long line to appease linter Signed-off-by: Nils Philippsen --- diff --git a/fedscm_admin/pagure.py b/fedscm_admin/pagure.py index 53e49f4..64241ff 100644 --- a/fedscm_admin/pagure.py +++ b/fedscm_admin/pagure.py @@ -101,7 +101,9 @@ def get_issues(): pagure_api_url = '{0}/api/0'.format(pagure_url) pagure_repo_issues_url = \ '{0}/releng/fedora-scm-requests/issues?{1}'.format( - pagure_api_url, urlencode({'status': 'Open', 'order_key': 'date_created', 'order': 'asc'})) + pagure_api_url, + urlencode({'status': 'Open', 'order_key': 'date_created', 'order': 'asc'}) + ) issues_rv = requests_wrapper( pagure_repo_issues_url, timeout=60, service_name='Pagure')