Skip to content
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.

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

⚡ slack-usergroups: performance fix #4800

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion reconcile/slack_usergroups.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,12 @@ def get_slack_usernames_from_owners(
else:
raise TypeError(f"{type(repo_cli)} not supported")

repo_owners = repo_owner_class(git_cli=repo_cli, ref=ref)
repo_owners = repo_owner_class(
git_cli=repo_cli,
ref=ref,
# we just need the root level OWNERS file
recursive=False,
)

try:
owners = repo_owners.get_root_owners()
Expand Down
3 changes: 3 additions & 0 deletions reconcile/utils/github_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ def __exit__(
) -> None:
self.cleanup()

def __str__(self) -> str:
return self._repo.html_url

def cleanup(self) -> None:
"""
Align with GitLabApi
Expand Down
3 changes: 3 additions & 0 deletions reconcile/utils/gitlab_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ def __enter__(self):
def __exit__(self, *exc):
self.cleanup()

def __str__(self):
return self.project.web_url

def cleanup(self):
"""
Close gl session.
Expand Down
27 changes: 13 additions & 14 deletions reconcile/utils/repo_owners.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ class RepoOwners:
Abstracts the owners of a repository with per-path granularity.
"""

def __init__(self, git_cli, ref="master"):
def __init__(self, git_cli, ref="master", recursive=True):
self._git_cli = git_cli
self._ref = ref
self._owners_map = None
self._recursive = recursive

@property
def owners_map(self):
Expand Down Expand Up @@ -116,33 +117,31 @@ def _get_owners_map(self):
:rtype: dict
"""
owners_map = {}
aliases = self._get_aliases()

repo_tree = self._git_cli.get_repository_tree(ref=self._ref)

aliases = None
owner_files = []
for item in repo_tree:
if item["path"] == "OWNERS_ALIASES":
aliases = self._get_aliases()
elif item["name"] == "OWNERS":
owner_files.append(item)
if self._recursive:
repo_tree = self._git_cli.get_repository_tree(ref=self._ref)
owner_files = [item for item in repo_tree if item["name"] == "OWNERS"]
else:
owner_files = [{"path": "OWNERS"}]
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if there is no such file? Won't we raise an exception later?

Copy link
Member Author

@chassing chassing Dec 18, 2024

Choose a reason for hiding this comment

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

I thought the same, but gitlab_api and github_api ignore any missing files. Nevertheless, I've added a warning in 1508ec and fixed an attribute error a long the way


for owner_file in owner_files:
raw_owners = self._git_cli.get_file(path=owner_file["path"], ref=self._ref)
if not raw_owners:
_LOG.warning(f"{self._git_cli!s}:{owner_file['path']} not found")
continue
try:
owners = yaml.safe_load(raw_owners.decode())
except yaml.parser.ParserError:
owners = None
if owners is None:
_LOG.warning(
"Non-parsable OWNERS file "
f"{self._git_cli.project.web_url}:{owner_file.get('path')}"
f"Non-parsable OWNERS file {self._git_cli!s}:{owner_file['path']}"
)
continue
if not isinstance(owners, dict):
_LOG.warning(
f"owner file {self._git_cli.project.web_url}:{owner_file.get('path')} "
"content is not a dictionary"
f"owner file {self._git_cli!s}:{owner_file['path']} content is not a dictionary"
)
continue

Expand Down