diff --git a/.github/workflows/health.yaml b/.github/workflows/health.yaml index b116f533..7fd81f38 100644 --- a/.github/workflows/health.yaml +++ b/.github/workflows/health.yaml @@ -28,7 +28,11 @@ name: Health # upload_coverage: false # flutter_packages: "pkgs/my_flutter_package" # ignore_license: "**.g.dart" +# ignore_changelog: "" # ignore_coverage: "**.mock.dart,**.g.dart" +# ignore_breaking: "" +# ignore_leaking: "" +# ignore_donotsubmit: "" # ignore_packages: "pkgs/helper_package" # checkout_submodules: false # experiments: "native-assets" @@ -97,11 +101,31 @@ on: default: "\"\"" required: false type: string + ignore_changelog: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string ignore_coverage: description: Which files to ignore for the coverage check. default: "\"\"" required: false type: string + ignore_breaking: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string + ignore_leaking: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string + ignore_donotsubmit: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string ignore_packages: description: Which packages to ignore. default: "\"\"" @@ -129,6 +153,7 @@ jobs: warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} flutter_packages: ${{ inputs.flutter_packages }} + ignore_changelog: ${{ inputs.ignore_changelog }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -173,6 +198,7 @@ jobs: warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} flutter_packages: ${{ inputs.flutter_packages }} + ignore_breaking: ${{ inputs.ignore_breaking }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -186,6 +212,7 @@ jobs: warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} flutter_packages: ${{ inputs.flutter_packages }} + ignore_donotsubmit: ${{ inputs.ignore_donotsubmit }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} @@ -199,6 +226,7 @@ jobs: warn_on: ${{ inputs.warn_on }} local_debug: ${{ inputs.local_debug }} flutter_packages: ${{ inputs.flutter_packages }} + ignore_leaking: ${{ inputs.ignore_leaking }} ignore_packages: ${{ inputs.ignore_packages }} checkout_submodules: ${{ inputs.checkout_submodules }} diff --git a/.github/workflows/health_base.yaml b/.github/workflows/health_base.yaml index 5c7e8b1d..a45abe51 100644 --- a/.github/workflows/health_base.yaml +++ b/.github/workflows/health_base.yaml @@ -62,11 +62,31 @@ on: default: "\"\"" required: false type: string + ignore_changelog: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string ignore_coverage: description: Which files to ignore for the coverage check. default: "\"\"" required: false type: string + ignore_breaking: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string + ignore_leaking: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string + ignore_donotsubmit: + description: Which files to ignore for the license check. + default: "\"\"" + required: false + type: string ignore_packages: description: Which packages to ignore. default: "\"\"" @@ -149,9 +169,13 @@ jobs: --fail_on ${{ inputs.fail_on }} \ --warn_on ${{ inputs.warn_on }} \ --flutter_packages ${{ inputs.flutter_packages }} \ + --ignore_packages ${{ inputs.ignore_packages }} \ --ignore_license ${{ inputs.ignore_license }} \ + --ignore_changelog ${{ inputs.ignore_changelog }} \ --ignore_coverage ${{ inputs.ignore_coverage }} \ - --ignore_packages ${{ inputs.ignore_packages }} \ + --ignore_breaking ${{ inputs.ignore_breaking }} \ + --ignore_leaking ${{ inputs.ignore_leaking }} \ + --ignore_donotsubmit ${{ inputs.ignore_donotsubmit }} \ --experiments ${{ inputs.experiments }} - run: test -f current_repo/output/comment.md || echo $'The ${{ inputs.check }} workflow has encountered an exception and did not complete.' >> current_repo/output/comment.md diff --git a/pkgs/firehose/CHANGELOG.md b/pkgs/firehose/CHANGELOG.md index 51cefaab..1d2a2705 100644 --- a/pkgs/firehose/CHANGELOG.md +++ b/pkgs/firehose/CHANGELOG.md @@ -1,6 +1,7 @@ ## 0.10.2-wip - Don't check licenses of generated files in PR health workflow. +- Add generalized ignore-per-checks to health workflow. ## 0.10.1 diff --git a/pkgs/firehose/README.md b/pkgs/firehose/README.md index fccca396..0997599b 100644 --- a/pkgs/firehose/README.md +++ b/pkgs/firehose/README.md @@ -229,17 +229,16 @@ jobs: | Name | Type | Description | Example | | ------------- | ------------- | ------------- | ------------- | -| checks | List of strings | What to check for in the PR health check | `"version,changelog,license,coverage,breaking,do-not-submit,leaking"` | -| fail_on | List of strings | Which checks should lead to failure | `"version,changelog,do-not-submit"` | -| warn_on | List of strings | Which checks should not fail, but only warn | `"license,coverage,breaking,leaking"` | -| upload_coverage | boolean | Whether to upload the coverage to [coveralls](https://coveralls.io/) | `true` | -| coverage_web | boolean | Whether to run `dart test -p chrome` for coverage | `false` | -| use-flutter | boolean | Whether to setup Flutter in this workflow | `false` | -| ignore_license | List of globs | | `"**.g.dart"` | -| ignore_coverage | List of globs | Which files to ignore for the license check | `"**.mock.dart,**.g.dart"` | -| ignore_packages | List of globs | Which packages to ignore | `"pkgs/helper_package"` | -| checkout_submodules | boolean | Whether to checkout submodules of git repositories | `false` | -| experiments | List of strings | Which experiments should be enabled for Dart | `"native-assets"` | +| `checks` | List of strings | What to check for in the PR health check | `"version,changelog,license,coverage,breaking,do-not-submit,leaking"` | +| `fail_on` | List of strings | Which checks should lead to failure | `"version,changelog,do-not-submit"` | +| `warn_on` | List of strings | Which checks should not fail, but only warn | `"license,coverage,breaking,leaking"` | +| `upload_coverage` | boolean | Whether to upload the coverage to [coveralls](https://coveralls.io/) | `true` | +| `coverage_web` | boolean | Whether to run `dart test -p chrome` for coverage | `false` | +| `flutter_packages` | List of strings | List of packages depending on Flutter | `"pkgs/intl_flutter"` | +| `ignore_*` | List of globs | Files to ignore, where `*` can be `license`, `changelog`, `coverage`, `breaking`, `leaking`, or `donotsubmit` | `"**.g.dart"` | +| `ignore_packages` | List of globs | Which packages to ignore completely | `"pkgs/helper_package"` | +| `checkout_submodules` | boolean | Whether to checkout submodules of git repositories | `false` | +| `experiments` | List of strings | Which experiments should be enabled for Dart | `"native-assets"` | ### Workflow docs diff --git a/pkgs/firehose/bin/health.dart b/pkgs/firehose/bin/health.dart index 122e0444..bb8799a2 100644 --- a/pkgs/firehose/bin/health.dart +++ b/pkgs/firehose/bin/health.dart @@ -9,7 +9,7 @@ import 'package:firehose/src/github.dart'; import 'package:firehose/src/health/health.dart'; void main(List arguments) async { - var checkTypes = Check.values.map((c) => c.name); + var checkTypes = Check.values.map((c) => c.displayName); var argParser = ArgParser() ..addOption( 'check', @@ -21,16 +21,6 @@ void main(List arguments) async { defaultsTo: [], help: 'Which packages to ignore.', ) - ..addMultiOption( - 'ignore_license', - defaultsTo: [], - help: 'Which files to ignore for the license check.', - ) - ..addMultiOption( - 'ignore_coverage', - defaultsTo: [], - help: 'Which files to ignore for the coverage check.', - ) ..addMultiOption( 'warn_on', allowed: checkTypes, @@ -54,15 +44,22 @@ void main(List arguments) async { defaultsTo: [], help: 'The Flutter packages in this repo', ); + for (var check in Check.values) { + argParser.addMultiOption( + 'ignore_${check.name}', + defaultsTo: [], + help: 'Which files to ignore for the ${check.displayName} check.', + ); + } final parsedArgs = argParser.parse(arguments); final checkStr = parsedArgs.option('check'); - final check = Check.values.firstWhere((c) => c.name == checkStr); + final check = Check.values.firstWhere((c) => c.displayName == checkStr); final warnOn = parsedArgs.multiOption('warn_on'); final failOn = parsedArgs.multiOption('fail_on'); final flutterPackages = _listNonEmpty(parsedArgs, 'flutter_packages'); final ignorePackages = _listNonEmpty(parsedArgs, 'ignore_packages'); - final ignoreLicense = _listNonEmpty(parsedArgs, 'ignore_license'); - final ignoreCoverage = _listNonEmpty(parsedArgs, 'ignore_coverage'); + final ignoredFor = Map.fromEntries(Check.values + .map((c) => MapEntry(c, _listNonEmpty(parsedArgs, 'ignore_${c.name}')))); final experiments = _listNonEmpty(parsedArgs, 'experiments'); final coverageWeb = parsedArgs.flag('coverage_web'); if (warnOn.toSet().intersection(failOn.toSet()).isNotEmpty) { @@ -76,8 +73,7 @@ void main(List arguments) async { failOn, coverageWeb, ignorePackages, - ignoreLicense, - ignoreCoverage, + ignoredFor, experiments, GithubApi(), flutterPackages, diff --git a/pkgs/firehose/lib/src/health/changelog.dart b/pkgs/firehose/lib/src/health/changelog.dart index e725ff5a..7324a422 100644 --- a/pkgs/firehose/lib/src/health/changelog.dart +++ b/pkgs/firehose/lib/src/health/changelog.dart @@ -12,13 +12,12 @@ import '../repo.dart'; Future>> packagesWithoutChangelog( GithubApi github, - List ignoredPackages, + List ignored, Directory directory, ) async { final repo = Repository(directory); - final packages = repo.locatePackages(ignore: ignoredPackages); - - final files = await github.listFilesForPR(directory, ignoredPackages); + final packages = repo.locatePackages(ignore: ignored); + final files = await github.listFilesForPR(directory, ignored); var packagesWithoutChangedChangelog = collectPackagesWithoutChangelogChanges( packages, diff --git a/pkgs/firehose/lib/src/health/coverage.dart b/pkgs/firehose/lib/src/health/coverage.dart index e128d611..4f2b447f 100644 --- a/pkgs/firehose/lib/src/health/coverage.dart +++ b/pkgs/firehose/lib/src/health/coverage.dart @@ -16,16 +16,14 @@ import 'lcov.dart'; class Coverage { final bool coverageWeb; - final List ignoredFiles; - final List ignoredPackages; + final List ignored; final Directory directory; final List experiments; final String dartExecutable; Coverage( this.coverageWeb, - this.ignoredFiles, - this.ignoredPackages, + this.ignored, this.directory, this.experiments, this.dartExecutable, @@ -33,7 +31,7 @@ class Coverage { CoverageResult compareCoveragesFor(List files, Directory base) { var repository = Repository(directory); - var packages = repository.locatePackages(ignore: ignoredPackages); + var packages = repository.locatePackages(ignore: ignored); print('Found packages $packages at $directory'); var filesOfInterest = files @@ -41,13 +39,12 @@ class Coverage { .where((file) => file.status != FileStatus.removed) .where((file) => isInSomePackage(packages, file.filename)) .where((file) => isNotATest(packages, file.filename)) - .where( - (file) => ignoredFiles.none((glob) => glob.matches(file.filename))) + .where((file) => ignored.none((glob) => glob.matches(file.filename))) .toList(); print('The files of interest are $filesOfInterest'); var baseRepository = Repository(base); - var basePackages = baseRepository.locatePackages(ignore: ignoredPackages); + var basePackages = baseRepository.locatePackages(ignore: ignored); print('Found packages $basePackages at $base'); var changedPackages = packages diff --git a/pkgs/firehose/lib/src/health/health.dart b/pkgs/firehose/lib/src/health/health.dart index 4b96b4f2..2b9d1027 100644 --- a/pkgs/firehose/lib/src/health/health.dart +++ b/pkgs/firehose/lib/src/health/health.dart @@ -29,9 +29,9 @@ enum Check { final String tag; - final String name; + final String displayName; - const Check(this.tag, this.name); + const Check(this.tag, this.displayName); } class Health { @@ -46,8 +46,7 @@ class Health { this.failOn, this.coverageweb, List ignoredPackages, - List ignoredLicense, - List ignoredCoverage, + Map> ignoredFor, this.experiments, this.github, List flutterPackages, { @@ -56,8 +55,8 @@ class Health { this.log = printLogger, }) : ignoredPackages = toGlobs(ignoredPackages), flutterPackageGlobs = toGlobs(flutterPackages), - ignoredFilesForCoverage = toGlobs(ignoredCoverage), - ignoredFilesForLicense = toGlobs(ignoredLicense), + ignoredFor = + ignoredFor.map((c, globString) => MapEntry(c, toGlobs(globString))), baseDirectory = base ?? Directory('../base_repo'), commentPath = comment ?? path.join( @@ -90,8 +89,7 @@ class Health { final List failOn; final bool coverageweb; final List ignoredPackages; - final List ignoredFilesForLicense; - final List ignoredFilesForCoverage; + final Map> ignoredFor; final List flutterPackageGlobs; final Directory baseDirectory; final List experiments; @@ -100,6 +98,8 @@ class Health { late final String dartExecutable; late final String? flutterExecutable; + List get ignored => [...ignoredPackages, ...ignoredFor[check] ?? []]; + String executable(bool isFlutter) => isFlutter ? flutterExecutable ?? dartExecutable : dartExecutable; @@ -109,25 +109,24 @@ class Health { if (!expectEnv(github.issueNumber?.toString(), 'ISSUE_NUMBER')) return; if (!expectEnv(github.sha, 'GITHUB_SHA')) return; - var checkName = check.name; + var checkName = check.displayName; log('Start health check for the check $checkName with'); log(' warnOn: $warnOn'); log(' failOn: $failOn'); log(' coverageweb: $coverageweb'); log(' flutterPackages: $flutterPackageGlobs'); log(' ignoredPackages: $ignoredPackages'); - log(' ignoredForLicense: $ignoredFilesForLicense'); - log(' ignoredForCoverage: $ignoredFilesForCoverage'); + log(' ignoredFor: $ignoredFor'); log(' baseDirectory: $baseDirectory'); log(' experiments: $experiments'); log('Checking for $checkName'); if (!github.prLabels.contains('skip-$checkName-check')) { final firstResult = await checkFor(check)(); final HealthCheckResult finalResult; - if (warnOn.contains(check.name) && + if (warnOn.contains(check.displayName) && firstResult.severity == Severity.error) { finalResult = firstResult.withSeverity(Severity.warning); - } else if (failOn.contains(check.name) && + } else if (failOn.contains(check.displayName) && firstResult.severity == Severity.warning) { finalResult = firstResult.withSeverity(Severity.error); } else { @@ -151,14 +150,13 @@ class Health { }; Future breakingCheck() async { - final filesInPR = await listFilesInPRorAll(ignoredPackages); + final filesInPR = await listFilesInPRorAll(); final changeForPackage = {}; final flutterPackages = packagesContaining(filesInPR, only: flutterPackageGlobs); log('This list of Flutter packages is $flutterPackages'); - for (var package - in packagesContaining(filesInPR, ignore: ignoredPackages)) { + for (var package in packagesContaining(filesInPR, ignore: ignored)) { log('Look for changes in $package'); var relativePath = path.relative(package.directory.path, from: directory.path); @@ -253,7 +251,7 @@ ${changeForPackage.entries.map((e) => '|${e.key.name}|${e.value.toMarkdownRow()} } Future leakingCheck() async { - var filesInPR = await listFilesInPRorAll(ignoredPackages); + var filesInPR = await listFilesInPRorAll(); final leaksForPackage = >{}; final flutterPackages = @@ -324,11 +322,8 @@ ${leaksForPackage.entries.map((e) => '|${e.key.name}|${e.value.join('
')}|'). } Future licenseCheck() async { - var files = await listFilesInPRorAll(ignoredPackages); - var allFilePaths = await getFilesWithoutLicenses( - directory, - [...ignoredFilesForLicense, ...ignoredPackages], - ); + var files = await listFilesInPRorAll(); + var allFilePaths = await getFilesWithoutLicenses(directory, ignored); var groupedPaths = allFilePaths.groupListsBy((filePath) { return files.any((f) => f.filename == filePath); @@ -380,7 +375,7 @@ ${unchangedFilesPaths.isNotEmpty ? unchangedMarkdown : ''} Future changelogCheck() async { var filePaths = await packagesWithoutChangelog( github, - ignoredPackages, + ignored, directory, ); @@ -405,7 +400,7 @@ Changes to files need to be [accounted for](https://github.com/dart-lang/ecosyst const supportedExtensions = ['.dart', '.json', '.md', '.txt']; final body = await github.pullrequestBody(); - var files = await listFilesInPRorAll(ignoredPackages); + var files = await listFilesInPRorAll(); log('Checking for DO_NOT${'_'}SUBMIT strings: $files'); final filesWithDNS = files .where((file) => @@ -436,31 +431,29 @@ ${filesWithDNS.map((e) => e.filename).map((e) => '|$e|').join('\n')} ); } - Future> listFilesInPRorAll(List ignore) async { - final files = await github.listFilesForPR(directory, ignore); - return healthYamlChanged(files) ? await _getAllFiles(ignore) : files; + Future> listFilesInPRorAll() async { + final files = await github.listFilesForPR(directory, ignored); + return healthYamlChanged(files) ? await _getAllFiles() : files; } - Future> _getAllFiles(List ignored) async => - await directory - .list(recursive: true) - .where((entity) => entity is File) - .map((file) => path.relative(file.path, from: directory.path)) - .where((file) => ignored.none((glob) => glob.matches(file))) - .map((file) => GitFile(file, FileStatus.added, directory)) - .toList(); + Future> _getAllFiles() async => await directory + .list(recursive: true) + .where((entity) => entity is File) + .map((file) => path.relative(file.path, from: directory.path)) + .where((file) => ignored.none((glob) => glob.matches(file))) + .map((file) => GitFile(file, FileStatus.added, directory)) + .toList(); Future coverageCheck() async { var coverage = Coverage( coverageweb, - ignoredFilesForCoverage, - ignoredPackages, + ignored, directory, experiments, dartExecutable, ); - var files = await listFilesInPRorAll(ignoredPackages); + var files = await listFilesInPRorAll(); var coverageResult = coverage.compareCoveragesFor(files, baseDirectory); var markdownResult = ''' @@ -495,7 +488,7 @@ This check for [test coverage](https://github.com/dart-lang/ecosystem/wiki/Test- $markdown -${isWorseThanInfo ? 'This check can be disabled by tagging the PR with `skip-${result.check.name}-check`.' : ''} +${isWorseThanInfo ? 'This check can be disabled by tagging the PR with `skip-${result.check.displayName}-check`.' : ''} '''; diff --git a/pkgs/firehose/lib/src/health/license.dart b/pkgs/firehose/lib/src/health/license.dart index c58b502f..375d6db0 100644 --- a/pkgs/firehose/lib/src/health/license.dart +++ b/pkgs/firehose/lib/src/health/license.dart @@ -14,7 +14,7 @@ final license = ''' // BSD-style license that can be found in the LICENSE file.'''; Future> getFilesWithoutLicenses( - Directory repositoryDir, List ignoredFiles) async { + Directory repositoryDir, List ignored) async { var dartFiles = await repositoryDir .list(recursive: true) .where((file) => file.path.endsWith('.dart')) @@ -23,7 +23,7 @@ Future> getFilesWithoutLicenses( var filesWithoutLicenses = dartFiles .map((file) { var relativePath = path.relative(file.path, from: repositoryDir.path); - if (ignoredFiles.none((glob) => + if (ignored.none((glob) => glob.matches(path.relative(file.path, from: repositoryDir.path)))) { var fileContents = File(file.path).readAsStringSync(); if (!fileIsGenerated(fileContents, file.path) && diff --git a/pkgs/firehose/test/coverage_test.dart b/pkgs/firehose/test/coverage_test.dart index 63abdbcf..964cf559 100644 --- a/pkgs/firehose/test/coverage_test.dart +++ b/pkgs/firehose/test/coverage_test.dart @@ -43,7 +43,7 @@ void main() { } class FakeHealth extends Coverage { - FakeHealth() : super(true, [], [], Directory.current, [], 'dart'); + FakeHealth() : super(true, [], Directory.current, [], 'dart'); @override Map getCoverage(Package? package) { diff --git a/pkgs/firehose/test/health_test.dart b/pkgs/firehose/test/health_test.dart index 7b76fbd0..d030a971 100644 --- a/pkgs/firehose/test/health_test.dart +++ b/pkgs/firehose/test/health_test.dart @@ -63,7 +63,7 @@ Future main() async { for (var check in Check.values) { test( - 'Check health workflow "${check.name}" against golden files', + 'Check health workflow "${check.displayName}" against golden files', () async => await checkGolden( check, fakeGithubApi([]), @@ -73,7 +73,7 @@ Future main() async { ); test( - 'Check health workflow "${check.name}" against golden files ' + 'Check health workflow "${check.displayName}" against golden files ' 'with health.yaml changed itself', () async => await checkGolden( check, @@ -96,7 +96,9 @@ Future main() async { fakeGithubApi([]), directory, suffix: '_ignore_license', - ignoredLicense: ['pkgs/package3/**'], + ignoreFor: { + Check.license: ['pkgs/package3/**'] + }, ); }); @@ -122,12 +124,12 @@ Future checkGolden( FakeGithubApi fakeGithubApi, Directory directory, { String suffix = '', - List ignoredLicense = const [], + Map> ignoreFor = const {}, List ignoredPackage = const [], List flutterPackages = const [], }) async { - final commentPath = p.join( - Directory.systemTemp.createTempSync().path, 'comment_${check.name}.md'); + final commentPath = p.join(Directory.systemTemp.createTempSync().path, + 'comment_${check.displayName}.md'); await FakeHealth( directory, check, @@ -135,8 +137,7 @@ Future checkGolden( [], false, ignoredPackage, - ignoredLicense, - [], + ignoreFor, [], fakeGithubApi, flutterPackages, @@ -145,8 +146,8 @@ Future checkGolden( log: printOnFailure, ).healthCheck(); var comment = await File(commentPath).readAsString(); - var goldenFile = - File(p.join('test_data', 'golden', 'comment_${check.name}$suffix.md')); + var goldenFile = File( + p.join('test_data', 'golden', 'comment_${check.displayName}$suffix.md')); if (Platform.environment['RESET_GOLDEN'] == '1') { goldenFile.writeAsStringSync(comment); } else { @@ -163,7 +164,6 @@ class FakeHealth extends Health { super.coverageweb, super.ignoredPackages, super.ignoredLicense, - super.ignoredCoverage, super.experiments, super.github, super.flutterPackages, {