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.

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

Jump to bottom

Use teams for ownership #142

Merged
merged 4 commits into from
Mar 13, 2024
Merged

Use teams for ownership #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a29ae75
Add tech-shared team for repo ownership
benbc Mar 8, 2024
813ca2d
Use GitHub teams for team ownership throughout instead of hard-coded …
benbc Mar 8, 2024
686a7d1
Explictly exclude archived repos for clarity
benbc Mar 8, 2024
4c6c4d5
Remove now-obsolete fine-grained calculation of metrics for archived …
benbc Mar 8, 2024
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
5 changes: 1 addition & 4 deletions metrics/github/prs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@ def calculate_counts(prs_by_repo, predicate):
for repo, prs in prs_by_repo.items():
for pr in prs:
start = pr["created_on"]
end = min(
repo.archived_on if repo.archived_on else date.today(),
pr["closed_on"] if pr["closed_on"] else date.today(),
)
end = pr["closed_on"] if pr["closed_on"] else date.today()
for day in iter_days(start, end):
if predicate(pr, day):
counts[(repo.org, repo.name, pr["author"], day)] += 1
Expand Down
7 changes: 2 additions & 5 deletions metrics/github/query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def repos(client, org):
org,
raw_repo["name"],
date_from_iso(raw_repo["createdAt"]),
date_from_iso(raw_repo["archivedAt"]),
raw_repo["archivedAt"] is not None,
raw_repo["hasVulnerabilityAlertsEnabled"],
)

Expand All @@ -42,12 +42,9 @@ class Repo:
org: str
name: str
created_on: date
archived_on: date | None
is_archived: bool = False
has_vulnerability_alerts_enabled: bool = False

def is_archived(self):
return bool(self.archived_on)


def team_repos(client, org, team):
"""The API doesn't make it easy for us to get all the information we need about repos in
Expand Down
126 changes: 9 additions & 117 deletions metrics/github/repos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@


def tech_repos(client, org):
return [r for r in query.repos(client, org) if _is_tech_owned(r)]
repo_names = []
for team in _TECH_TEAMS:
repo_names.extend(query.team_repos(client, org, team))

return [r for r in _active_repos(client, org) if r.name in repo_names]


def get_repo_ownership(client, orgs):
Expand All @@ -14,11 +18,7 @@ def get_repo_ownership(client, orgs):
for repo in query.team_repos(client, org, team):
ownership[repo] = team

active_repos = [
repo for repo in query.repos(client, org) if not repo.is_archived()
]

for repo in active_repos:
for repo in _active_repos(client, org):
if repo.name in ownership:
team = ownership[repo.name]
else:
Expand All @@ -28,117 +28,9 @@ def get_repo_ownership(client, orgs):
return repo_owners


def _is_tech_owned(repo):
# For now we are using a hard-coded list here because we don't have teams set up with repo
# lists for ebmdatalab. Later we can use the dynamically calculated repo ownership and get
# rid of this list.
#
# We use a deny-list rather than an allow-list so that newly created repos are treated as
# Tech-owned by default, in the hopes of minimizing surprise.
return not (repo.org in _NON_TECH_REPOS and repo.name in _NON_TECH_REPOS[repo.org])
def _active_repos(client, org):
return [repo for repo in query.repos(client, org) if not repo.is_archived]


# GitHub slugs for the teams we're interested in
_TECH_TEAMS = ["team-rap", "team-rex"]


_NON_TECH_REPOS = {
"ebmdatalab": [
"bennett-presentations",
"bnf-code-to-dmd",
"change_detection",
"copiloting",
"clinicaltrials-act-converter",
"clinicaltrials-act-tracker",
"copiloting-publication",
"datalab-jupyter",
"dmd-hosp-only",
"euctr-tracker-code",
"funding-applications",
"funding-report",
"ghost_branded_generics_paper",
"global-trial-landscape",
"imagemagick-magick",
"improvement_radar_prototype",
"jupyter-notebooks",
"kurtosis-pericyazine",
"lidocaine-eng-ire",
"low-priority-CCG-visit-RCT",
"nsaid-covid-codelist-notebook",
"opencorona-sandpit-for-fizz",
"opensafely-output-review",
"open-nhs-hospital-use-data",
"opioids-change-detection-notebook",
"outliers",
"prescribing-queries",
"price-concessions-accuracy-notebook",
"priceshocks",
"propaganda",
"publications",
"publications-copiloted",
"retracted.net",
"retractobot",
"retractobot-archive",
"rx-cost-item-analysis",
"Rx-Quantity-for-Long-Term-Conditions",
"Rx-Quantity-for-LTCs-notebook",
"scmd-narcolepsy",
"seb-test-notebook",
"service-analytics-team",
"teaching_resource",
"trialstracker",
"vaccinations-covid-codelist-notebook",
"nimodipine-rct",
"covid_trials_tracker-covid",
"datalabsupport",
"fdaaa_trends",
"openpathology-web",
"antibiotics-rct-analysis",
"seb-docker-test",
"openprescribing-doacs-workbook",
"sps-injectable-medicines-notebook",
"html-template-demo",
"covid-prescribing-impact-notebook",
"antibiotics-covid-codelist-notebook",
"medicines-seasonality-notebook",
"one-drug-database-analysis",
"cvd-covid-codelist-notebook",
"doacs-prescribing-notebook",
"ranitidine-shortages-notebook",
"raas-covid-codelist-notebook",
"statins-covid-codelist-notebook",
"medicines-and-poisoning-notebook",
"diabetes-drugs-covid-codelist-notebook",
"bnf-less-suitable-for-prescribing",
"insulin-covid-codelist-notebook",
"respiratory-meds-covid-codelist-notebook",
"immunosuppressant-covid-codelist-notebook",
"top-10-notebook",
"cusum-for-opioids-notebook",
"steroids-covid-codelist-notebook",
"chloroquine-covid-codelist-notebook",
"antibiotics-non-oral-routes-notebook",
"nhsdigital-shieldedrules-covid-codelist-methodology",
"anticoagulant-covid-codelist-notebook",
"repeat-prescribing-pandemic-covid-notebook",
"devon-formulary-adherence-notebook",
"statins-dose-paper",
"covid19_results_reporting",
"euctr_data_quality",
"factors-associated-with-changing-notebook",
"lidocaine-change-detection-notebook",
"datalab-notebook-template",
"antidepressant-trends",
"mortality_tracking-covid-notebook",
"ppi-covid-codelist-notebook",
"openpath-dash",
"fdaaa_requirements",
"openpath-pipeline",
"qof-data",
"nhstrusts",
],
"opensafely-core": [
"matching",
"scribe",
],
}
_TECH_TEAMS = ["team-rap", "team-rex", "tech-shared"]
3 changes: 1 addition & 2 deletions metrics/github/security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ def vulnerabilities(client, org, to_date):
for repo in repos.tech_repos(client, org):
vulns = list(map(Vulnerability.from_dict, query.vulnerabilities(client, repo)))

end = min(to_date, repo.archived_on) if repo.archived_on else to_date
for day in dates.iter_days(repo.created_on, end):
for day in dates.iter_days(repo.created_on, to_date):
closed_vulns = sum(1 for v in vulns if v.is_closed_on(day))
open_vulns = sum(1 for v in vulns if v.is_open_on(day))

Expand Down
23 changes: 2 additions & 21 deletions tests/metrics/github/test_prs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,6 @@ def test_makes_counts_for_every_day_between_pr_creation_and_now():
}


def test_drops_repos_after_archiving():
repo_ = repo(
"an-org",
"a-repo",
archived_on=YESTERDAY,
)
prs = {
repo_: [
pr(author="an-author", created_on=TWO_DAYS_AGO),
]
}

counts = calculate_counts(prs, true)
assert counts == {
("an-org", "a-repo", "an-author", TWO_DAYS_AGO): 1,
("an-org", "a-repo", "an-author", YESTERDAY): 1,
}


def test_counts_prs():
prs = {
repo("an-org", "a-repo"): [
Expand Down Expand Up @@ -144,12 +125,12 @@ def test_was_merged_in_on():
assert not was_merged_on(pr(merged_on=TOMORROW), TODAY)


def repo(org, name, archived_on=None):
def repo(org, name, is_archived=False):
return Repo(
org,
name,
created_on=date.min,
archived_on=archived_on,
is_archived=is_archived,
has_vulnerability_alerts_enabled=False,
)

Expand Down
86 changes: 69 additions & 17 deletions tests/metrics/github/test_repos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,33 +28,80 @@ def fake(_client, *keys):

def test_includes_tech_owned_repos(patch):
patch(
"repos", [repo("ebmdatalab", "sysadmin"), repo("opensafely-core", "job-server")]
"team_repos",
{
"opensafely-core": {
"team-rap": ["ehrql", "cohort-extractor"],
"team-rex": ["job-server"],
"tech-shared": [".github"],
}
},
)
assert len(repos.tech_repos(None, None)) == 2
patch(
"repos",
[
repo("opensafely-core", "ehrql"),
repo("opensafely-core", "cohort-extractor"),
repo("opensafely-core", "job-server"),
repo("opensafely-core", ".github"),
],
)
assert len(repos.tech_repos(None, "opensafely-core")) == 4


def test_excludes_non_tech_owned_repos(patch):
patch(
"team_repos",
{"opensafely-core": {"team-rap": [], "team-rex": [], "tech-shared": []}},
)
patch(
"repos",
[
repo("ebmdatalab", "clinicaltrials-act-tracker"),
repo("opensafely-core", "matching"),
repo("opensafely-core", "other-repo"),
],
)
assert len(repos.tech_repos(None, None)) == 0
assert len(repos.tech_repos(None, "opensafely-core")) == 0


def test_dont_exclude_repos_from_unknown_orgs(patch):
patch("repos", [repo("other", "any")])
assert len(repos.tech_repos(None, None)) == 1
def test_excludes_archived_repos(patch):
patch(
"team_repos",
{
"opensafely-core": {
"team-rap": ["other-repo"],
"team-rex": [],
"tech-shared": [],
}
},
)
patch(
"repos",
[
repo("opensafely-core", "other-repo", is_archived=True),
],
)
assert len(repos.tech_repos(None, "opensafely-core")) == 0


def test_looks_up_ownership(patch):
patch("repos", [repo("the_org", "repo1"), repo("the_org", "repo2")])
patch("team_repos", {"the_org": {"team-rex": ["repo1"], "team-rap": ["repo2"]}})
patch(
"repos",
[repo("the_org", "repo1"), repo("the_org", "repo2"), repo("the_org", "repo3")],
)
patch(
"team_repos",
{
"the_org": {
"team-rex": ["repo1"],
"team-rap": ["repo2"],
"tech-shared": ["repo3"],
}
},
)
assert repos.get_repo_ownership(None, ["the_org"]) == [
{"organisation": "the_org", "repo": "repo1", "owner": "team-rex"},
{"organisation": "the_org", "repo": "repo2", "owner": "team-rap"},
{"organisation": "the_org", "repo": "repo3", "owner": "tech-shared"},
]


Expand All @@ -63,8 +110,8 @@ def test_looks_up_ownership_across_orgs(patch):
patch(
"team_repos",
{
"org1": {"team-rex": ["repo1"], "team-rap": []},
"org2": {"team-rex": [], "team-rap": ["repo2"]},
"org1": {"team-rex": ["repo1"], "team-rap": [], "tech-shared": []},
"org2": {"team-rex": [], "team-rap": ["repo2"], "tech-shared": []},
},
)
assert repos.get_repo_ownership(None, ["org1", "org2"]) == [
Expand All @@ -74,24 +121,29 @@ def test_looks_up_ownership_across_orgs(patch):


def test_ignores_ownership_of_archived_repos(patch):
patch("repos", [repo("the_org", "the_repo", archived_on=date.min)])
patch("team_repos", {"the_org": {"team-rex": ["the_repo"], "team-rap": []}})
patch("repos", [repo("the_org", "the_repo", is_archived=True)])
patch(
"team_repos",
{"the_org": {"team-rex": ["the_repo"], "team-rap": [], "tech-shared": []}},
)
assert repos.get_repo_ownership(None, ["the_org"]) == []


def test_returns_none_for_unknown_ownership(patch):
patch("repos", [repo("the_org", "the_repo")])
patch("team_repos", {"the_org": {"team-rex": [], "team-rap": []}})
patch(
"team_repos", {"the_org": {"team-rex": [], "team-rap": [], "tech-shared": []}}
)
assert repos.get_repo_ownership(None, ["the_org"]) == [
{"organisation": "the_org", "repo": "the_repo", "owner": None}
]


def repo(org, name, archived_on=None):
def repo(org, name, is_archived=False):
return Repo(
org=org,
name=name,
created_on=date.min,
archived_on=archived_on,
is_archived=is_archived,
has_vulnerability_alerts_enabled=False,
)
Loading