From 32461a74cd84e3ba33266b3d9c932d233a6f3380 Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 1 Dec 2023 18:30:07 +0100 Subject: [PATCH] Upgrades to `health.yaml` (#201) * Continue `health.yaml` on error * Switch to package github * Fix tests * Rev version * Add test * Remove git dep on apitool * Fix DNS result * rename * Adapt markdown * Add failure step * Add more descriptive name * Update pkgs/firehose/CHANGELOG.md Co-authored-by: Devon Carew * Update pkgs/firehose/test/github_test.dart Co-authored-by: Devon Carew --------- Co-authored-by: Devon Carew --- .github/workflows/health.yaml | 14 +- pkgs/firehose/CHANGELOG.md | 5 + pkgs/firehose/bin/firehose.dart | 2 +- pkgs/firehose/lib/firehose.dart | 12 +- pkgs/firehose/lib/src/github.dart | 184 ++++++-------------- pkgs/firehose/lib/src/health/changelog.dart | 2 +- pkgs/firehose/lib/src/health/coverage.dart | 2 +- pkgs/firehose/lib/src/health/health.dart | 47 +++-- pkgs/firehose/lib/src/repo.dart | 2 +- pkgs/firehose/pubspec.yaml | 3 +- pkgs/firehose/test/coverage_test.dart | 4 +- pkgs/firehose/test/github_test.dart | 47 +++++ pkgs/firehose/test/repo_test.dart | 2 +- 13 files changed, 154 insertions(+), 172 deletions(-) create mode 100644 pkgs/firehose/test/github_test.dart diff --git a/.github/workflows/health.yaml b/.github/workflows/health.yaml index 8a9d450f..f5e816e1 100644 --- a/.github/workflows/health.yaml +++ b/.github/workflows/health.yaml @@ -92,15 +92,17 @@ jobs: run: dart pub global activate firehose if: ${{ !inputs.local_debug }} - - name: Install api_tool - run: dart pub global activate --source git https://github.com/bmw-tech/dart_apitool - - name: Install local firehose run: dart pub global activate --source path current_repo/pkgs/firehose/ if: ${{ inputs.local_debug }} - + + - name: Install api_tool + run: dart pub global activate dart_apitool + - name: Check PR health + id: healthstep if: ${{ github.event_name == 'pull_request' }} + continue-on-error: true # continue, so that the result is written as a comment. Fail in the last step env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} ISSUE_NUMBER: ${{ github.event.number }} @@ -125,3 +127,7 @@ jobs: with: name: output path: current_repo/output/ + + - name: Fail the workflow if "Check PR health" failed + if: steps.healthstep.outcome != 'success' + run: exit 1 diff --git a/pkgs/firehose/CHANGELOG.md b/pkgs/firehose/CHANGELOG.md index abf42074..0ce3eab4 100644 --- a/pkgs/firehose/CHANGELOG.md +++ b/pkgs/firehose/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.4.0 + +- Switch to using `package:github`. +- Check for `DO_NOT${'_'}SUBMIT` strings in the PR description. + ## 0.3.33 - Retry calls to pub.dev API. diff --git a/pkgs/firehose/bin/firehose.dart b/pkgs/firehose/bin/firehose.dart index 089648cd..d496bc1b 100644 --- a/pkgs/firehose/bin/firehose.dart +++ b/pkgs/firehose/bin/firehose.dart @@ -33,7 +33,7 @@ void main(List arguments) async { exit(1); } - final github = Github(); + final github = GithubApi(); if (publish && !github.inGithubContext) { _usage(argParser, error: 'Error: --publish can only be executed from within a GitHub ' diff --git a/pkgs/firehose/lib/firehose.dart b/pkgs/firehose/lib/firehose.dart index c6dfb44a..b1b107bd 100644 --- a/pkgs/firehose/lib/firehose.dart +++ b/pkgs/firehose/lib/firehose.dart @@ -34,12 +34,12 @@ class Firehose { /// - provide feedback on the PR (via a PR comment) about packages which are /// ready to publish Future validate() async { - var github = Github(); + var github = GithubApi(); // Do basic validation of our expected env var. if (!expectEnv(github.githubAuthToken, 'GITHUB_TOKEN')) return; - if (!expectEnv(github.repoSlug, 'GITHUB_REPOSITORY')) return; - if (!expectEnv(github.issueNumber, 'ISSUE_NUMBER')) return; + if (!expectEnv(github.repoSlug?.fullName, 'GITHUB_REPOSITORY')) return; + if (!expectEnv(github.issueNumber?.toString(), 'ISSUE_NUMBER')) return; if (!expectEnv(github.sha, 'GITHUB_SHA')) return; if ((github.actor ?? '').endsWith(_botSuffix)) { @@ -60,8 +60,6 @@ Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automati var existingCommentId = await allowFailure( github.findCommentId( - github.repoSlug!, - github.issueNumber!, user: _githubActionsUser, searchTerm: _publishBotTag, ), @@ -92,7 +90,7 @@ Saving existing comment id $existingCommentId to file ${idFile.path}'''); github.close(); } - Future verify(Github github) async { + Future verify(GithubApi github) async { var repo = Repository(); var packages = repo.locatePackages(); @@ -184,7 +182,7 @@ Saving existing comment id $existingCommentId to file ${idFile.path}'''); } Future _publish() async { - var github = Github(); + var github = GithubApi(); if (!expectEnv(github.refName, 'GITHUB_REF_NAME')) return false; diff --git a/pkgs/firehose/lib/src/github.dart b/pkgs/firehose/lib/src/github.dart index 6e57f909..cf1b31a7 100644 --- a/pkgs/firehose/lib/src/github.dart +++ b/pkgs/firehose/lib/src/github.dart @@ -3,33 +3,44 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -import 'dart:convert'; import 'dart:io'; -import 'package:http/http.dart' as http; +import 'package:github/github.dart'; import 'package:path/path.dart' as path; import 'repo.dart'; -// TODO:(devoncarew): Consider replacing some of this class with package:github. +class GithubApi { + final RepositorySlug? _repoSlug; + + final int? _issueNumber; -class Github { static Map get _env => Platform.environment; /// When true, details of any RPC error are printed to the console. final bool verbose; - http.Client? _httpClient; + GithubApi({this.verbose = false, RepositorySlug? repoSlug, int? issueNumber}) + : _repoSlug = repoSlug, + _issueNumber = issueNumber; - Github({this.verbose = false}); + late GitHub github = githubAuthToken != null + ? GitHub(auth: Authentication.withToken(githubAuthToken)) + : GitHub(); String? get githubAuthToken => _env['GITHUB_TOKEN']; /// The owner and repository name. For example, `octocat/Hello-World`. - String? get repoSlug => _env['GITHUB_REPOSITORY']; + RepositorySlug? get repoSlug { + return _repoSlug ?? + (_env['GITHUB_REPOSITORY'] != null + ? RepositorySlug.full(_env['GITHUB_REPOSITORY']!) + : null); + } /// The PR (or issue) number. - String? get issueNumber => _env['ISSUE_NUMBER']; + int? get issueNumber => + _issueNumber ?? int.tryParse(_env['ISSUE_NUMBER'] ?? ''); /// Any labels applied to this PR. List get prLabels => @@ -52,8 +63,6 @@ class Github { /// The ref name of the base where the PR branched off of. String? get baseRef => _env['base_ref']; - http.Client get httpClient => _httpClient ??= http.Client(); - /// Write the given [markdownSummary] content to the GitHub /// `GITHUB_STEP_SUMMARY` file. This will cause the markdown output to be /// appended to the GitHub job summary for the current PR. @@ -72,139 +81,45 @@ class Github { mode: FileMode.append); } - Future callRestApiGet(Uri uri) async { - var token = githubAuthToken!; - - return httpClient.get(uri, headers: { - 'Authorization': 'Bearer $token', - 'Accept': 'application/vnd.github+json', - }).then((response) { - return response.statusCode != 200 - ? throw RpcException(response.reasonPhrase!) - : response.body; - }); - } - - Future callRestApiPost(Uri uri, String body) async { - var token = githubAuthToken!; - - return httpClient - .post(uri, - headers: { - 'Authorization': 'Bearer $token', - 'Accept': 'application/vnd.github+json', - }, - body: body) - .then((response) { - if (response.statusCode != 201) { - if (verbose) { - stderr.writeln('${response.statusCode} ${response.reasonPhrase}'); - for (var entry in response.headers.entries) { - stderr.writeln('${entry.key}: ${entry.value}'); - } - stderr.writeln(response.body); - } - throw RpcException(response.reasonPhrase!); - } - return response.body; - }); - } - - Future callRestApiPatch(Uri uri, String body) async { - var token = githubAuthToken!; - - return httpClient - .patch(uri, - headers: { - 'Authorization': 'Bearer $token', - 'Accept': 'application/vnd.github+json', - }, - body: body) - .then((response) { - return response.statusCode != 200 - ? throw RpcException(response.reasonPhrase!) - : response.body; - }); - } - - Future callRestApiDelete(Uri uri) async { - var token = githubAuthToken!; - - return httpClient.delete(uri, headers: { - 'Authorization': 'Bearer $token', - 'Accept': 'application/vnd.github+json', - }).then((response) { - if (response.statusCode != 204) { - throw RpcException(response.reasonPhrase!); - } - }); - } - /// Find a comment on the PR matching the given criteria ([user], /// [searchTerm]). Return the issue ID if a matching comment is found or null /// if there's no match. - Future findCommentId( - String repoSlug, - String issueNumber, { + Future findCommentId({ required String user, String? searchTerm, }) async { - var result = await callRestApiGet( - Uri.parse('https://api.github.com/repos/$repoSlug/issues/$issueNumber/' - 'comments?per_page=100'), + final matchingComment = await github.issues + .listCommentsByIssue(repoSlug!, issueNumber!) + .map((comment) => comment) + .firstWhere( + (comment) { + final userMatch = comment?.user?.login == user; + final containsSearchTerm = searchTerm == null || + (comment?.body?.contains(searchTerm) ?? false); + return userMatch && containsSearchTerm; + }, + orElse: () => null, ); - - var items = jsonDecode(result) as List; - for (var item in items) { - item as Map; - var id = item['id'] as int; - var userLogin = (item['user'] as Map)['login'] as String; - var body = item['body'] as String; - - if (userLogin != user) continue; - if (searchTerm != null && !body.contains(searchTerm)) continue; - - return id; - } - - return null; + return matchingComment?.id; } - Future> listFilesForPR() async { - var result = await callRestApiGet( - Uri.parse( - 'https://api.github.com/repos/$repoSlug/pulls/$issueNumber/files'), - ); - var json = jsonDecode(result) as List; - var files = json - .map((e) => e as Map) - .map((e) => GitFile( - e['filename'] as String, - FileStatus.values.firstWhere( - (element) => element.name == e['status'] as String, - ), - )) - .toList(); - return files; - } - - void close() { - _httpClient?.close(); - } + Future> listFilesForPR() async => await github.pullRequests + .listFiles(repoSlug!, issueNumber!) + .map((prFile) => + GitFile(prFile.filename!, FileStatus.fromString(prFile.status!))) + .toList(); /// Write a notice message to the github log. void notice({required String message}) { print('::notice ::$message'); } -} - -class RpcException implements Exception { - final String message; - RpcException(this.message); + Future pullrequestBody() async { + final pullRequest = await github.pullRequests.get(repoSlug!, issueNumber!); + return pullRequest.body ?? ''; + } - @override - String toString() => 'RpcException: $message'; + void close() => github.dispose(); } class GitFile { @@ -223,6 +138,18 @@ class GitFile { @override String toString() => '$filename: $status'; + + @override + bool operator ==(Object other) { + if (identical(this, other)) return true; + + return other is GitFile && + other.filename == filename && + other.status == status; + } + + @override + int get hashCode => filename.hashCode ^ status.hashCode; } enum FileStatus { @@ -233,4 +160,7 @@ enum FileStatus { copied, changed, unchanged; + + static FileStatus fromString(String s) => + FileStatus.values.firstWhere((element) => element.name == s); } diff --git a/pkgs/firehose/lib/src/health/changelog.dart b/pkgs/firehose/lib/src/health/changelog.dart index 10534a03..a671433f 100644 --- a/pkgs/firehose/lib/src/health/changelog.dart +++ b/pkgs/firehose/lib/src/health/changelog.dart @@ -11,7 +11,7 @@ import '../repo.dart'; import '../utils.dart'; Future>> packagesWithoutChangelog( - Github github) async { + GithubApi github) async { final repo = Repository(); final packages = repo.locatePackages(); diff --git a/pkgs/firehose/lib/src/health/coverage.dart b/pkgs/firehose/lib/src/health/coverage.dart index 8fdbff2d..1050a677 100644 --- a/pkgs/firehose/lib/src/health/coverage.dart +++ b/pkgs/firehose/lib/src/health/coverage.dart @@ -17,7 +17,7 @@ class Coverage { Coverage(this.coverageWeb); - Future compareCoverages(Github github) async { + Future compareCoverages(GithubApi github) async { var files = await github.listFilesForPR(); var basePath = '../base_repo/'; diff --git a/pkgs/firehose/lib/src/health/health.dart b/pkgs/firehose/lib/src/health/health.dart index c49ba5b7..774c4dfe 100644 --- a/pkgs/firehose/lib/src/health/health.dart +++ b/pkgs/firehose/lib/src/health/health.dart @@ -44,12 +44,12 @@ class Health { Health(this.directory); Future healthCheck(List args, bool coverageweb) async { - var github = Github(); + var github = GithubApi(); // Do basic validation of our expected env var. if (!expectEnv(github.githubAuthToken, 'GITHUB_TOKEN')) return; - if (!expectEnv(github.repoSlug, 'GITHUB_REPOSITORY')) return; - if (!expectEnv(github.issueNumber, 'ISSUE_NUMBER')) return; + if (!expectEnv(github.repoSlug?.fullName, 'GITHUB_REPOSITORY')) return; + if (!expectEnv(github.issueNumber?.toString(), 'ISSUE_NUMBER')) return; if (!expectEnv(github.sha, 'GITHUB_SHA')) return; if ((github.actor ?? '').endsWith(_botSuffix)) { @@ -70,7 +70,7 @@ class Health { changelogCheck, if (args.contains('coverage') && !github.prLabels.contains('skip-coverage-check')) - (Github github) => coverageCheck(github, coverageweb), + (GithubApi github) => coverageCheck(github, coverageweb), if (args.contains('breaking') && !github.prLabels.contains('skip-breaking-check')) breakingCheck, @@ -82,11 +82,9 @@ class Health { var checked = await Future.wait(checks.map((check) => check(github)).toList()); await writeInComment(github, checked); - - github.close(); } - Future validateCheck(Github github) async { + Future validateCheck(GithubApi github) async { //TODO: Add Flutter support for PR health checks var results = await Firehose(directory, false).verify(github); @@ -106,7 +104,7 @@ Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automati ); } - Future breakingCheck(Github github) async { + Future breakingCheck(GithubApi github) async { final repo = Repository(); final packages = repo.locatePackages(); var changeForPackage = {}; @@ -180,7 +178,7 @@ ${changeForPackage.entries.map((e) => '|${e.key.name}|${e.value.toMarkdownRow()} ); } - Future licenseCheck(Github github) async { + Future licenseCheck(GithubApi github) async { var files = await github.listFilesForPR(); var allFilePaths = await getFilesWithoutLicenses(Directory.current); @@ -224,7 +222,7 @@ ${unchangedFilesPaths.isNotEmpty ? unchangedMarkdown : ''} ); } - Future changelogCheck(Github github) async { + Future changelogCheck(GithubApi github) async { var filePaths = await packagesWithoutChangelog(github); final markdownResult = ''' @@ -243,10 +241,11 @@ Changes to files need to be [accounted for](https://github.com/dart-lang/ecosyst ); } - Future doNotSubmitCheck(Github github) async { + Future doNotSubmitCheck(GithubApi github) async { + final body = await github.pullrequestBody(); final files = await github.listFilesForPR(); print('Checking for DO_NOT${'_'}SUBMIT strings: $files'); - var filesWithDNS = files + final filesWithDNS = files .where((file) => ![FileStatus.removed, FileStatus.unchanged].contains(file.status)) .where((file) => File(file.relativePath) @@ -254,22 +253,28 @@ Changes to files need to be [accounted for](https://github.com/dart-lang/ecosyst .contains('DO_NOT${'_'}SUBMIT')) .toList(); print('Found files with DO_NOT_${'SUBMIT'}: $filesWithDNS'); + + final bodyContainsDNS = body.contains('DO_NOT${'_'}SUBMIT'); + print('The body contains a DO_NOT${'_'}SUBMIT string: $bodyContainsDNS'); final markdownResult = ''' +Body contains `DO_NOT${'_'}SUBMIT`: $bodyContainsDNS + | Files with `DO_NOT_${'SUBMIT'}` | | :--- | ${filesWithDNS.map((e) => e.filename).map((e) => '|$e|').join('\n')} '''; + final hasDNS = filesWithDNS.isNotEmpty || bodyContainsDNS; return HealthCheckResult( 'do-not-submit', _doNotSubmitBotTag, - filesWithDNS.isNotEmpty ? Severity.error : Severity.success, - filesWithDNS.isNotEmpty ? markdownResult : null, + hasDNS ? Severity.error : Severity.success, + hasDNS ? markdownResult : null, ); } Future coverageCheck( - Github github, + GithubApi github, bool coverageWeb, ) async { var coverage = await Coverage(coverageWeb).compareCoverages(github); @@ -293,7 +298,7 @@ This check for [test coverage](https://github.com/dart-lang/ecosystem/wiki/Test- } Future writeInComment( - Github github, + GithubApi github, List results, ) async { var commentText = @@ -318,16 +323,8 @@ ${isWorseThanInfo ? 'This check can be disabled by tagging the PR with `skip-${r var summary = '$_prHealthTag\n\n$commentText'; github.appendStepSummary(summary); - var repoSlug = github.repoSlug!; - var issueNumber = github.issueNumber!; - var existingCommentId = await allowFailure( - github.findCommentId( - repoSlug, - issueNumber, - user: _githubActionsUser, - searchTerm: _prHealthTag, - ), + github.findCommentId(user: _githubActionsUser, searchTerm: _prHealthTag), logError: print, ); diff --git a/pkgs/firehose/lib/src/repo.dart b/pkgs/firehose/lib/src/repo.dart index 0ca96acb..ad433d8f 100644 --- a/pkgs/firehose/lib/src/repo.dart +++ b/pkgs/firehose/lib/src/repo.dart @@ -74,7 +74,7 @@ class Repository { } } - Uri calculateReleaseUri(Package package, Github github) { + Uri calculateReleaseUri(Package package, GithubApi github) { final tag = calculateRepoTag(package); final title = 'package:${package.name} v${package.pubspec.version}'; final body = package.changelog.describeLatestChanges; diff --git a/pkgs/firehose/pubspec.yaml b/pkgs/firehose/pubspec.yaml index 28902f9f..322a78e2 100644 --- a/pkgs/firehose/pubspec.yaml +++ b/pkgs/firehose/pubspec.yaml @@ -1,6 +1,6 @@ name: firehose description: A tool to automate publishing of Pub packages from GitHub actions. -version: 0.3.33 +version: 0.4.0 repository: https://github.com/dart-lang/ecosystem/tree/main/pkgs/firehose environment: @@ -13,6 +13,7 @@ executables: dependencies: args: ^2.3.0 collection: ^1.17.2 + github: ^9.20.0 http: ^0.13.0 path: ^1.8.0 pub_semver: ^2.1.0 diff --git a/pkgs/firehose/test/coverage_test.dart b/pkgs/firehose/test/coverage_test.dart index b384f06c..24e53f9d 100644 --- a/pkgs/firehose/test/coverage_test.dart +++ b/pkgs/firehose/test/coverage_test.dart @@ -32,9 +32,7 @@ void main() { }); test('Compare coverage', () async { var coverages = FakeHealth().compareCoveragesFor( - [ - GitFile('testfile.dart', FileStatus.modified), - ], + [GitFile('testfile.dart', FileStatus.modified)], 'base_path_does_not_exist', ); diff --git a/pkgs/firehose/test/github_test.dart b/pkgs/firehose/test/github_test.dart new file mode 100644 index 00000000..ef47b54e --- /dev/null +++ b/pkgs/firehose/test/github_test.dart @@ -0,0 +1,47 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:firehose/src/github.dart'; +import 'package:github/github.dart'; +import 'package:test/test.dart'; + +Future main() async { + var github = GithubApi( + repoSlug: RepositorySlug('dart-lang', 'ecosystem'), + issueNumber: 148, + ); + test('Fetching pull request description', () async { + var pullrequestDescription = await github.pullrequestBody(); + expect( + pullrequestDescription, + startsWith( + 'Bumps [actions/labeler](https://github.com/actions/labeler) from 4.0.4 to 4.3.0.\n')); + }); + test('Listing files for PR', () async { + var files = await github.listFilesForPR(); + expect(files, [ + GitFile('.github/workflows/pull_request_label.yml', FileStatus.modified), + ]); + }); + test('Find comment', () async { + var commentId = await github.findCommentId(user: 'auto-submit[bot]'); + expect(commentId, 1660891263); + }); + test('Find comment with searchterm', () async { + var commentId = await github.findCommentId( + user: 'auto-submit[bot]', + searchTerm: 'before re-applying this label.', + ); + expect(commentId, 1660891263); + }); + test('Find comment with searchterm', () async { + var commentId = await github.findCommentId( + user: 'auto-submit[bot]', + searchTerm: 'some string not in the comment', + ); + expect(commentId, isNull); + }); + + tearDownAll(() => github.close()); +} diff --git a/pkgs/firehose/test/repo_test.dart b/pkgs/firehose/test/repo_test.dart index 34e93aea..dabc58e4 100644 --- a/pkgs/firehose/test/repo_test.dart +++ b/pkgs/firehose/test/repo_test.dart @@ -27,7 +27,7 @@ void main() { }); test('github release link', () { - final github = Github(); + final github = GithubApi(); final package = packages.locatePackages().single; final releaseUri = packages.calculateReleaseUri(package, github); expect(releaseUri.path, '/${github.repoSlug}/releases/new');