From 35c04cec97dbbda52a4b7abb3154b552a8e2c616 Mon Sep 17 00:00:00 2001 From: Max Mehl Date: Wed, 21 Aug 2024 16:31:49 +0200 Subject: [PATCH] improve handling of empty owner configuration This keeps backward compatibility. Also refactor a bit to make code easier to read --- gh_org_mgr/_gh_org.py | 125 +++++++++++++++++++++++------------------- gh_org_mgr/manage.py | 2 + 2 files changed, 72 insertions(+), 55 deletions(-) diff --git a/gh_org_mgr/_gh_org.py b/gh_org_mgr/_gh_org.py index 1b2c7f5..9ba895a 100644 --- a/gh_org_mgr/_gh_org.py +++ b/gh_org_mgr/_gh_org.py @@ -5,7 +5,6 @@ """Class for the GitHub organization which contains most of the logic""" import logging -import sys from dataclasses import asdict, dataclass, field from github import Github, UnknownObjectException @@ -187,19 +186,29 @@ def _get_current_org_owners(self) -> None: for member in self.org.get_members(role="admin"): self.current_org_owners.append(member) - def _get_configured_org_owners(self, cfg_org_owners: list[str]) -> None: + def _get_configured_org_owners(self, cfg_org_owners: list[str] | str | None) -> None: """Import configured owners for the organization from the org configuration""" - # Sanity check - if not cfg_org_owners or not isinstance(cfg_org_owners, list): - logging.critical( - "No owners for GitHub organisation defined or not as list. " - "This would make the organization unmanageable. Will not continue." + # Add configured owners if they are a list + if isinstance(cfg_org_owners, list): + # Import users to dataclass attribute, lower-case + for user in cfg_org_owners: + self.configured_org_owners.append(user.lower()) + else: + logging.warning( + "The organisation owners are not configured as a proper list. Will not handle them." + ) + + def _check_non_empty_configured_owners(self) -> bool: + """Handle if there are no configured owners. Returns True if owners are configured.""" + if not self.configured_org_owners: + logging.warning( + "No owners for your GitHub organisation configured. Will not make any " + "change regarding the ownership, and continue with the current owners: %s", + ", ".join([user.login for user in self.current_org_owners]), ) - sys.exit(1) + return False - # Import users to dataclass attribute, lower-case - for user in cfg_org_owners: - self.configured_org_owners.append(user.lower()) + return True def _is_user_authenticated_user(self, user: NamedUser) -> bool: """Check if a given NamedUser is the authenticated user""" @@ -213,58 +222,64 @@ def sync_org_owners(self, cfg_org_owners: list, dry: bool = False, force: bool = self._get_current_org_owners() self._get_configured_org_owners(cfg_org_owners=cfg_org_owners) + # Abort owner synchronisation if there are no configured owners + if not self._check_non_empty_configured_owners(): + return + + # Get differences between the current and configured owners + owners_remove, owners_ok, owners_add = self.compare_two_lists( + self.configured_org_owners, [user.login for user in self.current_org_owners] + ) # Compare configured (lower-cased) owners with lower-cased list of current owners - current_org_owners_str = [user.login for user in self.current_org_owners] - if self.configured_org_owners == current_org_owners_str: + if not owners_remove and not owners_add: logging.info("Organization owners are in sync, no changes") - else: - # Get differences between the two lists - remove, match, add = self.compare_two_lists( - self.configured_org_owners, current_org_owners_str - ) - logging.debug( - "Organization owners are not in sync. Config: '%s' vs. Current: '%s'", - self.configured_org_owners, - current_org_owners_str, - ) - logging.debug("Will remove %s, will not change %s, will add %s", remove, match, add) + return - # Add the missing owners - for user in add: - if gh_user := self._resolve_gh_username(user, ""): - logging.info("Adding user '%s' as organization owner", gh_user.login) - if not dry: - self.org.add_to_members(gh_user, "admin") + logging.debug( + "Organization owners are not in sync. Config: '%s' vs. Current: '%s'", + self.configured_org_owners, + self.current_org_owners, + ) + logging.debug( + "Will remove %s, will not change %s, will add %s", owners_remove, owners_ok, owners_add + ) - # Remove the surplus owners - for user in remove: - if gh_user := self._resolve_gh_username(user, ""): - logging.info( - "User '%s' is not configured as organization owners. " - "Will make them a normal member", + # Add the missing owners + for user in owners_add: + if gh_user := self._resolve_gh_username(user, ""): + logging.info("Adding user '%s' as organization owner", gh_user.login) + if not dry: + self.org.add_to_members(gh_user, "admin") + + # Remove the surplus owners + for user in owners_remove: + if gh_user := self._resolve_gh_username(user, ""): + logging.info( + "User '%s' is not configured as organization owners. " + "Will make them a normal member", + gh_user.login, + ) + # Handle authenticated user being the same as the one you want to degrade + if self._is_user_authenticated_user(gh_user): + logging.warning( + "The user '%s' you want to remove from owners is the one you " + "authenticated with. This may disrupt all further operations. " + "Unless you run the program with --force, " + "this operation will not be executed.", gh_user.login, ) - # Handle authenticated user being the same as the one you want to degrade - if self._is_user_authenticated_user(gh_user): - logging.warning( - "The user '%s' you want to remove from owners is the one you " - "authenticated with. This may disrupt all further operations. " - "Unless you run the program with --force, " - "this operation will not be executed.", - gh_user.login, + # Check if user forced this operation + if force: + logging.info( + "You called the program with --force, " + "so it will remove yourself from the owners" ) - # Check if user forced this operation - if force: - logging.info( - "You called the program with --force, " - "so it will remove yourself from the owners" - ) - else: - continue + else: + continue - # Execute the degradation of the owner - if not dry: - self.org.add_to_members(gh_user, "member") + # Execute the degradation of the owner + if not dry: + self.org.add_to_members(gh_user, "member") # Update the current organisation owners self._get_current_org_owners() diff --git a/gh_org_mgr/manage.py b/gh_org_mgr/manage.py index 7d8f478..b97a339 100644 --- a/gh_org_mgr/manage.py +++ b/gh_org_mgr/manage.py @@ -94,6 +94,8 @@ def main(): if args.command == "sync": if args.dry: logging.info("Dry-run mode activated, will not make any changes at GitHub") + if args.force: + logging.info("Force mode activated, will make potentially dangerous actions") org = GHorg()