From d3d7b58c3743b6146b5fb818508d9eda958e8cd9 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Tue, 3 Sep 2019 11:33:29 +0200 Subject: [PATCH 1/5] ci: ensure all tool maintainers are assignable on issues GitHub only allows people explicitly listed as collaborators on the repository or who commented on the issue/PR to be assignees, failing to create the issue if non-assignable people are assigned. This adds an extra check on CI to make sure all the people listed as tool maintainers can be assigned to toolstate issues. The check won't be executed on PR builds due to the lack of a valid token. --- src/ci/azure-pipelines/steps/run.yml | 7 ++++ src/tools/publish_toolstate.py | 60 ++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/ci/azure-pipelines/steps/run.yml b/src/ci/azure-pipelines/steps/run.yml index ac6b344a45e66..da0a899ac85eb 100644 --- a/src/ci/azure-pipelines/steps/run.yml +++ b/src/ci/azure-pipelines/steps/run.yml @@ -147,8 +147,15 @@ steps: git clone --depth=1 https://github.com/rust-lang-nursery/rust-toolstate.git cd rust-toolstate python2.7 "$BUILD_SOURCESDIRECTORY/src/tools/publish_toolstate.py" "$(git rev-parse HEAD)" "$(git log --format=%s -n1 HEAD)" "" "" + # Only check maintainers if this build is supposed to publish toolstate. + # Builds that are not supposed to publish don't have the access token. + if [ -n "${TOOLSTATE_PUBLISH+is_set}" ]; then + TOOLSTATE_VALIDATE_MAINTAINERS_REPO=rust-lang/rust python2.7 "${BUILD_SOURCESDIRECTORY}/src/tools/publish_toolstate.py" + fi cd .. rm -rf rust-toolstate + env: + TOOLSTATE_REPO_ACCESS_TOKEN: $(TOOLSTATE_REPO_ACCESS_TOKEN) condition: and(succeeded(), not(variables.SKIP_JOB), eq(variables['IMAGE'], 'mingw-check')) displayName: Verify the publish_toolstate script works diff --git a/src/tools/publish_toolstate.py b/src/tools/publish_toolstate.py index 2e2505b7f0246..bc00fbc90bf96 100755 --- a/src/tools/publish_toolstate.py +++ b/src/tools/publish_toolstate.py @@ -7,6 +7,8 @@ ## It is set as callback for `src/ci/docker/x86_64-gnu-tools/repo.sh` by the CI scripts ## when a new commit lands on `master` (i.e., after it passed all checks on `auto`). +from __future__ import print_function + import sys import re import os @@ -20,6 +22,8 @@ import urllib.request as urllib2 # List of people to ping when the status of a tool or a book changed. +# These should be collaborators of the rust-lang/rust repository (with at least +# read privileges on it). CI will fail otherwise. MAINTAINERS = { 'miri': '@oli-obk @RalfJung @eddyb', 'clippy-driver': '@Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995 @yaahc', @@ -52,6 +56,53 @@ } +def validate_maintainers(repo, github_token): + '''Ensure all maintainers are assignable on a GitHub repo''' + next_link_re = re.compile(r'<([^>]+)>; rel="next"') + + # Load the list of assignable people in the GitHub repo + assignable = [] + url = 'https://api.github.com/repos/%s/collaborators?per_page=100' % repo + while url is not None: + response = urllib2.urlopen(urllib2.Request(url, headers={ + 'Authorization': 'token ' + github_token, + # Properly load nested teams. + 'Accept': 'application/vnd.github.hellcat-preview+json', + })) + for user in json.loads(response.read()): + assignable.append(user['login']) + # Load the next page if available + if 'Link' in response.headers: + matches = next_link_re.match(response.headers['Link']) + if matches is not None: + url = matches.group(1) + else: + url = None + + errors = False + for tool, maintainers in MAINTAINERS.items(): + for maintainer in maintainers.split(' '): + if maintainer.startswith('@'): + maintainer = maintainer[1:] + if maintainer not in assignable: + errors = True + print( + "error: %s maintainer @%s is not assignable in the %s repo" + % (tool, maintainer, repo), + ) + + if errors: + print() + print(" To be assignable, a person needs to be explicitly listed as a") + print(" collaborator in the repository settings. The simple way to") + print(" fix this is to ask someone with 'admin' privileges on the repo") + print(" to add the person or whole team as a collaborator with 'read'") + print(" privileges. Those privileges don't grant any extra permissions") + print(" so it's safe to apply them.") + print() + print("The build will fail due to this.") + exit(1) + def read_current_status(current_commit, path): '''Reads build status of `current_commit` from content of `history/*.tsv` ''' @@ -200,6 +251,15 @@ def update_latest( if __name__ == '__main__': + if 'TOOLSTATE_VALIDATE_MAINTAINERS_REPO' in os.environ: + repo = os.environ['TOOLSTATE_VALIDATE_MAINTAINERS_REPO'] + if 'TOOLSTATE_REPO_ACCESS_TOKEN' in os.environ: + github_token = os.environ['TOOLSTATE_REPO_ACCESS_TOKEN'] + validate_maintainers(repo, github_token) + else: + print('skipping toolstate maintainers validation since no GitHub token is present') + exit(0) + cur_commit = sys.argv[1] cur_datetime = datetime.datetime.utcnow().strftime('%Y-%m-%dT%H:%M:%SZ') cur_commit_msg = sys.argv[2] From eb97b1bfdede51981dbed39bb3fe483311f8f65d Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Tue, 3 Sep 2019 12:04:22 +0200 Subject: [PATCH 2/5] ci: rename Gankro to Gankra in toolstate --- src/tools/publish_toolstate.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/publish_toolstate.py b/src/tools/publish_toolstate.py index bc00fbc90bf96..cd7a182da2706 100755 --- a/src/tools/publish_toolstate.py +++ b/src/tools/publish_toolstate.py @@ -30,7 +30,7 @@ 'rls': '@Xanewok', 'rustfmt': '@topecongiro', 'book': '@carols10cents @steveklabnik', - 'nomicon': '@frewsxcv @Gankro', + 'nomicon': '@frewsxcv @Gankra', 'reference': '@steveklabnik @Havvy @matthewjasper @ehuss', 'rust-by-example': '@steveklabnik @marioidival @projektir', 'embedded-book': ( From f968c1a4f51b1e57f1f613055332b3bf63f5864e Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 4 Sep 2019 10:08:54 +0200 Subject: [PATCH 3/5] ci: address publish_toolstate review comments --- src/tools/publish_toolstate.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/tools/publish_toolstate.py b/src/tools/publish_toolstate.py index cd7a182da2706..5211d1141c7e0 100755 --- a/src/tools/publish_toolstate.py +++ b/src/tools/publish_toolstate.py @@ -69,15 +69,14 @@ def validate_maintainers(repo, github_token): # Properly load nested teams. 'Accept': 'application/vnd.github.hellcat-preview+json', })) - for user in json.loads(response.read()): - assignable.append(user['login']) + assignable.extend(user['login'] for user in json.load(response)) # Load the next page if available - if 'Link' in response.headers: - matches = next_link_re.match(response.headers['Link']) + url = None + link_header = response.headers.get('Link') + if link_header: + matches = next_link_re.match(link_header) if matches is not None: url = matches.group(1) - else: - url = None errors = False for tool, maintainers in MAINTAINERS.items(): @@ -251,13 +250,14 @@ def update_latest( if __name__ == '__main__': - if 'TOOLSTATE_VALIDATE_MAINTAINERS_REPO' in os.environ: - repo = os.environ['TOOLSTATE_VALIDATE_MAINTAINERS_REPO'] - if 'TOOLSTATE_REPO_ACCESS_TOKEN' in os.environ: - github_token = os.environ['TOOLSTATE_REPO_ACCESS_TOKEN'] + repo = os.environ.get('TOOLSTATE_VALIDATE_MAINTAINERS_REPO') + if repo: + github_token = os.environ.get('TOOLSTATE_REPO_ACCESS_TOKEN') + if github_token: validate_maintainers(repo, github_token) else: print('skipping toolstate maintainers validation since no GitHub token is present') + # When validating maintainers don't run the full script. exit(0) cur_commit = sys.argv[1] From f2576c821d2e15b23dcea8af9b3ed08cf4a5795f Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 4 Sep 2019 10:21:23 +0200 Subject: [PATCH 4/5] ci: convert maintainer list in publish_toolstate to a set --- src/tools/publish_toolstate.py | 45 +++++++++++++++++----------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/src/tools/publish_toolstate.py b/src/tools/publish_toolstate.py index 5211d1141c7e0..217f8a7f6f4c7 100755 --- a/src/tools/publish_toolstate.py +++ b/src/tools/publish_toolstate.py @@ -25,20 +25,23 @@ # These should be collaborators of the rust-lang/rust repository (with at least # read privileges on it). CI will fail otherwise. MAINTAINERS = { - 'miri': '@oli-obk @RalfJung @eddyb', - 'clippy-driver': '@Manishearth @llogiq @mcarton @oli-obk @phansch @flip1995 @yaahc', - 'rls': '@Xanewok', - 'rustfmt': '@topecongiro', - 'book': '@carols10cents @steveklabnik', - 'nomicon': '@frewsxcv @Gankra', - 'reference': '@steveklabnik @Havvy @matthewjasper @ehuss', - 'rust-by-example': '@steveklabnik @marioidival @projektir', - 'embedded-book': ( - '@adamgreig @andre-richter @jamesmunns @korken89 ' - '@ryankurte @thejpster @therealprof' - ), - 'edition-guide': '@ehuss @Centril @steveklabnik', - 'rustc-guide': '@mark-i-m @spastorino @amanjeev' + 'miri': {'oli-obk', 'RalfJung', 'eddyb'}, + 'clippy-driver': { + 'Manishearth', 'llogiq', 'mcarton', 'oli-obk', 'phansch', 'flip1995', + 'yaahc', + }, + 'rls': {'Xanewok'}, + 'rustfmt': {'topecongiro'}, + 'book': {'carols10cents', 'steveklabnik'}, + 'nomicon': {'frewsxcv', 'Gankra'}, + 'reference': {'steveklabnik', 'Havvy', 'matthewjasper', 'ehuss'}, + 'rust-by-example': {'steveklabnik', 'marioidival', 'projektir'}, + 'embedded-book': { + 'adamgreig', 'andre-richter', 'jamesmunns', 'korken89', + 'ryankurte', 'thejpster', 'therealprof', + }, + 'edition-guide': {'ehuss', 'Centril', 'steveklabnik'}, + 'rustc-guide': {'mark-i-m', 'spastorino', 'amanjeev'}, } REPOS = { @@ -80,9 +83,7 @@ def validate_maintainers(repo, github_token): errors = False for tool, maintainers in MAINTAINERS.items(): - for maintainer in maintainers.split(' '): - if maintainer.startswith('@'): - maintainer = maintainer[1:] + for maintainer in maintainers: if maintainer not in assignable: errors = True print( @@ -123,13 +124,12 @@ def maybe_delink(message): def issue( tool, status, - maintainers, + assignees, relevant_pr_number, relevant_pr_user, pr_reviewer, ): # Open an issue about the toolstate failure. - assignees = [x.strip() for x in maintainers.split('@') if x != ''] if status == 'test-fail': status_description = 'has failing tests' else: @@ -150,7 +150,7 @@ def issue( REPOS.get(tool), relevant_pr_user, pr_reviewer )), 'title': '`{}` no longer builds after {}'.format(tool, relevant_pr_number), - 'assignees': assignees, + 'assignees': list(assignees), 'labels': ['T-compiler', 'I-nominated'], }) print("Creating issue:\n{}".format(request)) @@ -200,18 +200,19 @@ def update_latest( old = status[os] new = s.get(tool, old) status[os] = new + maintainers = ' '.join('@'+name for name in MAINTAINERS[tool]) if new > old: # comparing the strings, but they are ordered appropriately! # things got fixed or at least the status quo improved changed = True message += '🎉 {} on {}: {} → {} (cc {}, @rust-lang/infra).\n' \ - .format(tool, os, old, new, MAINTAINERS.get(tool)) + .format(tool, os, old, new, maintainers) elif new < old: # tests or builds are failing and were not failing before changed = True title = '💔 {} on {}: {} → {}' \ .format(tool, os, old, new) message += '{} (cc {}, @rust-lang/infra).\n' \ - .format(title, MAINTAINERS.get(tool)) + .format(title, maintainers) # Most tools only create issues for build failures. # Other failures can be spurious. if new == 'build-fail' or (tool == 'miri' and new == 'test-fail'): From ce451b9b269207feb565f566cdbd34d7ec369b4b Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Mon, 9 Sep 2019 16:03:57 +0200 Subject: [PATCH 5/5] ci: remove projektir from toolstate notifications They don't contribute to rust-by-example anymore. --- src/tools/publish_toolstate.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/publish_toolstate.py b/src/tools/publish_toolstate.py index 217f8a7f6f4c7..4060b90d952bd 100755 --- a/src/tools/publish_toolstate.py +++ b/src/tools/publish_toolstate.py @@ -35,7 +35,7 @@ 'book': {'carols10cents', 'steveklabnik'}, 'nomicon': {'frewsxcv', 'Gankra'}, 'reference': {'steveklabnik', 'Havvy', 'matthewjasper', 'ehuss'}, - 'rust-by-example': {'steveklabnik', 'marioidival', 'projektir'}, + 'rust-by-example': {'steveklabnik', 'marioidival'}, 'embedded-book': { 'adamgreig', 'andre-richter', 'jamesmunns', 'korken89', 'ryankurte', 'thejpster', 'therealprof',