Skip to content

Commit

Permalink
Add ignore flag to health workflows (#218)
Browse files Browse the repository at this point in the history
* Add `ignore` flag to health workflows

* Switch to glob

* Fix multiline

* Add default glob

* Delete multiline

* Fix errors

* Fix health commenting

* Propagate ignore

* Switch to specific ignores

* Add defaults

* Add test repos

* Start adding health tests

* Add golden files

* Fix changelog

* Fix coverage

* Fix breaking

* More fixes

* Merge

* Fixes

* Move

* Fix tests

* Ignore test data licenses

* Add golden without license

* Fix license

* Rename

* Fix

* Fix analyze issue

* Add args printing

* Add defaults

* Add more defaults

* Remove empty

* Match recursive

* Add docs

* Add link to docs
  • Loading branch information
mosuem authored Jan 29, 2024
1 parent a283d70 commit 9ee08a4
Show file tree
Hide file tree
Showing 24 changed files with 454 additions and 98 deletions.
33 changes: 31 additions & 2 deletions .github/workflows/health.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# A CI configuration to check PR health.
# A CI configuration to check PR health. Check the docs at https://github.com/dart-lang/ecosystem/tree/main/pkgs/firehose#health.

name: Health

Expand Down Expand Up @@ -27,7 +27,13 @@ name: Health
# coverage_web: false
# upload_coverage: false
# use-flutter: true

# use-flutter: true
# use-flutter: true
# ignore_license: "**.g.dart"
# ignore_coverage: "**.mock.dart,**.g.dart"
# ignore_packages: "pkgs/helper_package"
# checkout_submodules: false
# experiments: "native-assets"

on:
workflow_call:
Expand Down Expand Up @@ -82,6 +88,21 @@ on:
default: false
required: false
type: boolean
ignore_license:
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_packages:
description: Which packages to ignore.
default: "\"\""
required: false
type: string
checkout_submodules:
description: Whether to checkout submodules of git repositories.
default: false
Expand All @@ -104,6 +125,7 @@ jobs:
warn_on: ${{ inputs.warn_on }}
local_debug: ${{ inputs.local_debug }}
use-flutter: ${{ inputs.use-flutter }}
ignore_packages: ${{ inputs.ignore_packages }}
checkout_submodules: ${{ inputs.checkout_submodules }}

changelog:
Expand All @@ -116,6 +138,7 @@ jobs:
warn_on: ${{ inputs.warn_on }}
local_debug: ${{ inputs.local_debug }}
use-flutter: ${{ inputs.use-flutter }}
ignore_packages: ${{ inputs.ignore_packages }}
checkout_submodules: ${{ inputs.checkout_submodules }}

license:
Expand All @@ -128,6 +151,8 @@ jobs:
warn_on: ${{ inputs.warn_on }}
local_debug: ${{ inputs.local_debug }}
use-flutter: ${{ inputs.use-flutter }}
ignore_license: ${{ inputs.ignore_license }}
ignore_packages: ${{ inputs.ignore_packages }}
checkout_submodules: ${{ inputs.checkout_submodules }}

coverage:
Expand All @@ -142,6 +167,8 @@ jobs:
coverage_web: ${{ inputs.coverage_web }}
local_debug: ${{ inputs.local_debug }}
use-flutter: ${{ inputs.use-flutter }}
ignore_coverage: ${{ inputs.ignore_coverage }}
ignore_packages: ${{ inputs.ignore_packages }}
checkout_submodules: ${{ inputs.checkout_submodules }}
experiments: ${{ inputs.experiments }}

Expand All @@ -155,6 +182,7 @@ jobs:
warn_on: ${{ inputs.warn_on }}
local_debug: ${{ inputs.local_debug }}
use-flutter: ${{ inputs.use-flutter }}
ignore_packages: ${{ inputs.ignore_packages }}
checkout_submodules: ${{ inputs.checkout_submodules }}

do-not-submit:
Expand All @@ -167,6 +195,7 @@ jobs:
warn_on: ${{ inputs.warn_on }}
local_debug: ${{ inputs.local_debug }}
use-flutter: ${{ inputs.use-flutter }}
ignore_packages: ${{ inputs.ignore_packages }}
checkout_submodules: ${{ inputs.checkout_submodules }}

comment:
Expand Down
18 changes: 18 additions & 0 deletions .github/workflows/health_base.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,21 @@ on:
default: false
required: false
type: boolean
ignore_license:
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_packages:
description: Which packages to ignore.
default: "\"\""
required: false
type: string
checkout_submodules:
description: Whether to checkout submodules of git repositories.
default: false
Expand Down Expand Up @@ -124,6 +139,9 @@ jobs:
${{ fromJSON('{"true":"--coverage_web","false":""}')[inputs.coverage_web] }} \
--fail_on ${{ inputs.fail_on }} \
--warn_on ${{ inputs.warn_on }} \
--ignore_license ${{ inputs.ignore_license }} \
--ignore_coverage ${{ inputs.ignore_coverage }} \
--ignore_packages ${{ inputs.ignore_packages }} \
--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
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/health_internal.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,5 @@ jobs:
checks: version,changelog,license,coverage,breaking,do-not-submit
fail_on: version,changelog,do-not-submit
warn_on: license,coverage,breaking
ignore_license: 'pkgs/firehose/test_data'
ignore_coverage: 'pkgs/firehose/bin'
4 changes: 4 additions & 0 deletions pkgs/firehose/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.6.1

- Add `ignore` flags to the health workflow.

## 0.6.0

- Make the health workflow testable with golden tests.
Expand Down
24 changes: 21 additions & 3 deletions pkgs/firehose/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,12 @@ This is a Github workflow to check PR health.

When run from a PR, this tool will check a configurable subset of the following

* If the package versioning is correct and consistent, see `firehose` description above.
* If a changelog entry has been added.
* If all `.dart` files have a license header.
* The package versioning is correct and consistent, see `firehose` description above.
* A changelog entry has been added.
* All `.dart` files have a license header.
* How the test coverage is affected by the PR.
* The package versioning takes into account any breaking changes in the PR.
* The PR contains `DO_NOT_SUBMIT` strings in the files or the description.

This tool can work with either single package repos or with mono-repos (repos
containing several packages).
Expand All @@ -186,6 +188,22 @@ jobs:
# checks: "version,changelog,license,coverage"
```

### Options

| 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"` |
| 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"` |
| 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"` |

### Workflow docs

The description of the common workflow for repos using this tool can be found at
Expand Down
38 changes: 30 additions & 8 deletions pkgs/firehose/bin/health.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,21 @@ void main(List<String> arguments) async {
allowed: checkTypes,
help: 'Check PR health.',
)
..addMultiOption(
'ignore_packages',
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,
Expand All @@ -33,14 +48,15 @@ void main(List<String> arguments) async {
'coverage_web',
help: 'Whether to run web tests for coverage',
);
var parsedArgs = argParser.parse(arguments);
var check = parsedArgs['check'] as String;
var warnOn = parsedArgs['warn_on'] as List<String>;
var failOn = parsedArgs['fail_on'] as List<String>;
var experiments = (parsedArgs['experiments'] as List<String>)
.where((name) => name.isNotEmpty)
.toList();
var coverageWeb = parsedArgs['coverage_web'] as bool;
final parsedArgs = argParser.parse(arguments);
final check = parsedArgs['check'] as String;
final warnOn = parsedArgs['warn_on'] as List<String>;
final failOn = parsedArgs['fail_on'] as List<String>;
final ignorePackages = _listNonEmpty(parsedArgs, 'ignore_packages');
final ignoreLicense = _listNonEmpty(parsedArgs, 'ignore_license');
final ignoreCoverage = _listNonEmpty(parsedArgs, 'ignore_coverage');
final experiments = _listNonEmpty(parsedArgs, 'experiments');
final coverageWeb = parsedArgs['coverage_web'] as bool;
if (warnOn.toSet().intersection(failOn.toSet()).isNotEmpty) {
throw ArgumentError('The checks for which warnings are displayed and the '
'checks which lead to failure must be disjoint.');
Expand All @@ -51,7 +67,13 @@ void main(List<String> arguments) async {
warnOn,
failOn,
coverageWeb,
ignorePackages,
ignoreLicense,
ignoreCoverage,
experiments,
GithubApi(),
).healthCheck();
}

List<String> _listNonEmpty(ArgResults parsedArgs, String key) =>
(parsedArgs[key] as List<String>).where((e) => e.isNotEmpty).toList();
9 changes: 7 additions & 2 deletions pkgs/firehose/lib/firehose.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import 'dart:io';
import 'dart:math';

import 'package:glob/glob.dart';

import 'src/github.dart';
import 'src/pub.dart';
import 'src/repo.dart';
Expand Down Expand Up @@ -90,9 +92,12 @@ Saving existing comment id $existingCommentId to file ${idFile.path}''');
github.close();
}

Future<VerificationResults> verify(GithubApi github) async {
Future<VerificationResults> verify(
GithubApi github, [
List<Glob> ignoredPackages = const [],
]) async {
var repo = Repository(directory);
var packages = repo.locatePackages();
var packages = repo.locatePackages(ignoredPackages);

var pub = Pub();

Expand Down
14 changes: 10 additions & 4 deletions pkgs/firehose/lib/src/github.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

import 'dart:io';

import 'package:collection/collection.dart';
import 'package:github/github.dart';
import 'package:glob/glob.dart';
import 'package:http/http.dart' as http;
import 'package:path/path.dart' as path;

Expand Down Expand Up @@ -107,14 +109,19 @@ class GithubApi {
return matchingComment?.id;
}

Future<List<GitFile>> listFilesForPR(Directory directory) async =>
Future<List<GitFile>> listFilesForPR(
Directory directory, [
List<Glob> ignoredFiles = const [],
]) async =>
await _github.pullRequests
.listFiles(repoSlug!, issueNumber!)
.map((prFile) => GitFile(
prFile.filename!,
FileStatus.fromString(prFile.status!),
directory,
))
.where((file) =>
ignoredFiles.none((glob) => glob.matches(file.filename)))
.toList();

/// Write a notice message to the github log.
Expand All @@ -135,9 +142,8 @@ class GitFile {
final FileStatus status;
final Directory directory;

bool isInPackage(Package package) {
return path.isWithin(package.directory.path, pathInRepository);
}
bool isInPackage(Package package) =>
path.isWithin(package.directory.path, pathInRepository);

String get pathInRepository => path.join(directory.path, filename);

Expand Down
6 changes: 4 additions & 2 deletions pkgs/firehose/lib/src/health/changelog.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,21 @@

import 'dart:io';

import 'package:glob/glob.dart';
import 'package:path/path.dart' as path;

import '../github.dart';
import '../repo.dart';

Future<Map<Package, List<GitFile>>> packagesWithoutChangelog(
GithubApi github,
List<Glob> ignoredPackages,
Directory directory,
) async {
final repo = Repository(directory);
final packages = repo.locatePackages();
final packages = repo.locatePackages(ignoredPackages);

final files = await github.listFilesForPR(directory);
final files = await github.listFilesForPR(directory, ignoredPackages);

var packagesWithoutChangedChangelog = collectPackagesWithoutChangelogChanges(
packages,
Expand Down
Loading

0 comments on commit 9ee08a4

Please sign in to comment.