diff --git a/packages/shorebird_ci/CHANGELOG.md b/packages/shorebird_ci/CHANGELOG.md index 38ba06b1..07346e8a 100644 --- a/packages/shorebird_ci/CHANGELOG.md +++ b/packages/shorebird_ci/CHANGELOG.md @@ -1,5 +1,10 @@ +# 0.2.1 + +- Static main workflow YAML map keys default to each package's `name:` and fall back to `_` only when two or more packages share a name. Avoids prefixing in the common case while still handling duplicate-`name:` repos. The `package_name:` input passed to the reusable workflow keeps the actual package name for codecov flag display. +- Static reusable workflows now gate the test and codecov-upload steps on a new `has_unit_tests` input. Packages without a `test/` directory no longer attempt to run `dart test` / `flutter test` or upload coverage. The input defaults to `true` for back-compat with workflows that don't pass it. + # 0.2.0 - Generated dynamic workflow now pins `fetch-depth: 0` on the setup-job checkout. Without it, `affected_packages` fails on every PR because the default shallow checkout doesn't include `origin/main`. The static main workflow gets the same fix so dorny/paths-filter can diff on push events. diff --git a/packages/shorebird_ci/lib/src/commands/generate_command.dart b/packages/shorebird_ci/lib/src/commands/generate_command.dart index ffb4b303..0694ecac 100644 --- a/packages/shorebird_ci/lib/src/commands/generate_command.dart +++ b/packages/shorebird_ci/lib/src/commands/generate_command.dart @@ -7,6 +7,7 @@ import 'package:shorebird_ci/src/commands/repo_root_option.dart'; import 'package:shorebird_ci/src/dependency_resolver.dart'; import 'package:shorebird_ci/src/flutter_version_resolver.dart'; import 'package:shorebird_ci/src/package_description.dart'; +import 'package:shorebird_ci/src/package_slug.dart'; import 'package:shorebird_ci/src/repository_analyzer.dart'; import 'package:shorebird_ci/src/repository_description.dart'; @@ -209,6 +210,11 @@ updates: required List packages, }) { final resolver = DependencyResolver(repository.root.path); + final slugs = computePackageSlugs( + packages: packages, + repoRoot: repository.root.path, + ); + final buffer = StringBuffer() ..write(''' # Generated by shorebird_ci --style static. Safe to edit. @@ -230,10 +236,8 @@ jobs: '''); for (final package in packages) { - buffer.writeln( - ' ${package.name}: ' - '\${{ steps.filter.outputs.${package.name} }}', - ); + final slug = slugs[package]!; + buffer.writeln(' $slug: \${{ steps.filter.outputs.$slug }}'); } // Verify first so we fail fast if the dorny filters below have @@ -261,8 +265,9 @@ jobs: from: repository.root.path, ); final sortedDeps = resolver.resolve(packageDir).toList()..sort(); + final slug = slugs[package]!; - buffer.writeln(' ${package.name}:'); + buffer.writeln(' $slug:'); for (final dep in sortedDeps) { buffer.writeln(' - $dep/**'); } @@ -285,16 +290,18 @@ jobs: final reusable = isFlutter ? '_shorebird_ci_flutter.yaml' : '_shorebird_ci_dart.yaml'; + final slug = slugs[package]!; buffer.write(''' - ${package.name}: + $slug: needs: changes - if: needs.changes.outputs.${package.name} == 'true' + if: needs.changes.outputs.$slug == 'true' uses: ./.github/workflows/$reusable with: package_name: ${package.name} package_path: $packageDir has_bloc_lint: ${RepositoryAnalyzer.dependsOnBlocLint(root: package.root)} + has_unit_tests: ${RepositoryAnalyzer.hasUnitTests(root: package.root)} subpackages: "${subpackages.join(' ')}" '''); @@ -324,18 +331,21 @@ jobs: final testStep = hasCodecov ? r''' - name: Run Tests + if: inputs.has_unit_tests working-directory: ${{ inputs.package_path }} run: | dart pub global activate coverage && \ dart test --coverage=coverage && \ dart pub global run coverage:format_coverage --lcov --in=coverage --out=coverage/lcov.info --report-on=lib --check-ignore - - uses: codecov/codecov-action@v5 + - if: inputs.has_unit_tests + uses: codecov/codecov-action@v5 with: flags: ${{ inputs.package_name }} working-directory: ${{ inputs.package_path }} ''' : r''' - - working-directory: ${{ inputs.package_path }} + - if: inputs.has_unit_tests + working-directory: ${{ inputs.package_path }} run: dart test '''; @@ -355,6 +365,10 @@ on: required: false default: false type: boolean + has_unit_tests: + required: false + default: true + type: boolean subpackages: required: false default: "" @@ -392,15 +406,18 @@ $testStep'''; String _buildFlutterReusableWorkflow({required bool hasCodecov}) { final testStep = hasCodecov ? r''' - - working-directory: ${{ inputs.package_path }} + - if: inputs.has_unit_tests + working-directory: ${{ inputs.package_path }} run: flutter test --coverage - - uses: codecov/codecov-action@v5 + - if: inputs.has_unit_tests + uses: codecov/codecov-action@v5 with: flags: ${{ inputs.package_name }} working-directory: ${{ inputs.package_path }} ''' : r''' - - working-directory: ${{ inputs.package_path }} + - if: inputs.has_unit_tests + working-directory: ${{ inputs.package_path }} run: flutter test '''; @@ -428,6 +445,10 @@ on: required: false default: false type: boolean + has_unit_tests: + required: false + default: true + type: boolean subpackages: required: false default: "" diff --git a/packages/shorebird_ci/lib/src/commands/verify_command.dart b/packages/shorebird_ci/lib/src/commands/verify_command.dart index e257c4af..1c61b3ec 100644 --- a/packages/shorebird_ci/lib/src/commands/verify_command.dart +++ b/packages/shorebird_ci/lib/src/commands/verify_command.dart @@ -6,6 +6,7 @@ import 'package:shorebird_ci/src/commands/repo_root_option.dart'; import 'package:shorebird_ci/src/dependency_resolver.dart'; import 'package:shorebird_ci/src/dorny_filter.dart'; import 'package:shorebird_ci/src/package_description.dart'; +import 'package:shorebird_ci/src/package_slug.dart'; import 'package:shorebird_ci/src/repository_analyzer.dart'; /// Marker comment that the `generate` command writes into dynamic @@ -21,9 +22,11 @@ const dynamicCoverageMarker = '# shorebird_ci-managed: dynamic'; /// - **Dynamic**: a workflow that calls `shorebird_ci affected_packages` /// covers every package automatically. One dynamic workflow means no /// missing packages. -/// - **Static**: each package name appears in a `dorny/paths-filter` -/// block somewhere. Missing packages are reported with the dorny -/// entry that should be added (including transitive deps). +/// - **Static**: each package's slug (see [computePackageSlugs]) appears +/// in a `dorny/paths-filter` block somewhere. For most packages the +/// slug is just the package name; when two packages share a name the +/// slug is `_`. Missing packages are reported with +/// the dorny entry that should be added (including transitive deps). class VerifyCommand extends Command with RepoRootOption { /// Creates a [VerifyCommand]. VerifyCommand() { @@ -98,15 +101,19 @@ class VerifyCommand extends Command with RepoRootOption { return 0; } + final slugs = computePackageSlugs( + packages: allPackages, + repoRoot: repoRoot, + ); final missing = []; for (final pkg in allPackages) { if (ignoreSet.contains(pkg.name)) continue; - final workflows = coverageMap[pkg.name]; + final slug = slugs[pkg]!; + final workflows = coverageMap[slug]; if (workflows != null) { - stdout.writeln( - 'OK: ${pkg.name} (${workflows.join(', ')})', - ); + final label = slug == pkg.name ? pkg.name : '${pkg.name} ($slug)'; + stdout.writeln('OK: $label (${workflows.join(', ')})'); } else { missing.add(pkg); } @@ -124,11 +131,13 @@ class VerifyCommand extends Command with RepoRootOption { final packageDir = posixRelative(pkg.rootPath, from: repoRoot); final deps = resolver.resolve(packageDir); final sortedDeps = deps.toList()..sort(); + final slug = slugs[pkg]!; + final label = slug == pkg.name ? pkg.name : '${pkg.name} ($slug)'; stdout - ..writeln('MISSING: ${pkg.name}') + ..writeln('MISSING: $label') ..writeln(' Add this entry to a dorny paths-filter block:') - ..writeln(' ${pkg.name}:'); + ..writeln(' $slug:'); for (final dep in sortedDeps) { stdout.writeln(' - $dep/**'); } diff --git a/packages/shorebird_ci/lib/src/package_slug.dart b/packages/shorebird_ci/lib/src/package_slug.dart new file mode 100644 index 00000000..b89abd89 --- /dev/null +++ b/packages/shorebird_ci/lib/src/package_slug.dart @@ -0,0 +1,50 @@ +import 'package:shorebird_ci/src/package_description.dart'; +import 'package:shorebird_ci/src/repository_analyzer.dart'; + +/// Maps each package to the YAML map key used for its dorny filter, +/// changes-job output, and per-package job in the static main workflow. +/// +/// A package's slug defaults to its `name:`. When two or more packages +/// share a `name:`, those packages fall back to `_` +/// to disambiguate. Parent dirs are normalized to pub's +/// `[a-z][a-z0-9_]*` rules so the slug can be used in GitHub Actions +/// expressions. `generate` writes these slugs and `verify` reads them, +/// so both commands must agree on the rule. +Map computePackageSlugs({ + required List packages, + required String repoRoot, +}) { + final byName = >{}; + for (final pkg in packages) { + byName.putIfAbsent(pkg.name, () => []).add(pkg); + } + final duplicateNames = { + for (final entry in byName.entries) + if (entry.value.length > 1) entry.key, + }; + + return { + for (final pkg in packages) + pkg: _slugFor( + package: pkg, + repoRoot: repoRoot, + duplicateNames: duplicateNames, + ), + }; +} + +String _slugFor({ + required PackageDescription package, + required String repoRoot, + required Set duplicateNames, +}) { + if (!duplicateNames.contains(package.name)) return package.name; + final relative = posixRelative(package.rootPath, from: repoRoot); + final parts = relative.split('/'); + if (parts.length < 2) return package.name; + final parent = parts[parts.length - 2].toLowerCase().replaceAll( + RegExp('[^a-z0-9_]'), + '_', + ); + return '${parent}_${package.name}'; +} diff --git a/packages/shorebird_ci/lib/src/repository_analyzer.dart b/packages/shorebird_ci/lib/src/repository_analyzer.dart index e724efdc..81bef4d4 100644 --- a/packages/shorebird_ci/lib/src/repository_analyzer.dart +++ b/packages/shorebird_ci/lib/src/repository_analyzer.dart @@ -87,8 +87,6 @@ class RepositoryAnalyzer { .whereType() .toList(); - _checkForDuplicateNames(packageDescriptions); - return RepositoryDescription( packages: packageDescriptions, root: repositoryRoot, @@ -97,29 +95,6 @@ class RepositoryAnalyzer { ); } - static void _checkForDuplicateNames( - List packages, - ) { - final byName = >{}; - for (final pkg in packages) { - byName.putIfAbsent(pkg.name, () => []).add(pkg); - } - final duplicates = byName.entries.where((e) => e.value.length > 1); - if (duplicates.isEmpty) return; - - final buffer = StringBuffer( - 'Duplicate package names found. Each package in the workspace ' - 'must have a unique `name:` in its pubspec.yaml.\n', - ); - for (final entry in duplicates) { - buffer.writeln(' ${entry.key}:'); - for (final pkg in entry.value) { - buffer.writeln(' - ${pkg.rootPath}'); - } - } - throw StateError(buffer.toString().trimRight()); - } - /// Allowed characters in a package or subpackage path: letters, /// digits, `_`, `-`, `.`, and `/`. Any other character (whitespace, /// shell metacharacters, Unicode) is rejected. @@ -144,10 +119,9 @@ class RepositoryAnalyzer { /// Pub's own naming convention for packages: lowercase letter or /// underscore start, then lowercase letters, digits, and underscores. - /// `pubspec.yaml` is just YAML — pub doesn't gate this name until you - /// publish — so a malformed name can land here and get embedded as - /// a YAML map key in the generated workflow. Validate at analysis - /// time instead. + /// `pubspec.yaml` is just YAML, and pub doesn't gate this name until + /// publish. A malformed name flows into the slug used as a YAML map + /// key in the generated workflow, so validate at analysis time. static final _safePackageNameRegex = RegExp(r'^[a-z][a-z0-9_]*$'); static void _requireSafePackageName(String name, {required String source}) { diff --git a/packages/shorebird_ci/pubspec.yaml b/packages/shorebird_ci/pubspec.yaml index a159800d..7793be90 100644 --- a/packages/shorebird_ci/pubspec.yaml +++ b/packages/shorebird_ci/pubspec.yaml @@ -3,7 +3,7 @@ description: >- CI tooling for Dart and Flutter monorepos. Generates GitHub Actions workflows, resolves affected packages via dependency graphs, and verifies path filters stay in sync. -version: 0.2.0 +version: 0.2.1 homepage: https://shorebird.dev repository: https://github.com/shorebirdtech/shorebird/tree/main/packages/shorebird_ci topics: [ci, github-actions, monorepo, shorebird] diff --git a/packages/shorebird_ci/test/generate_command_test.dart b/packages/shorebird_ci/test/generate_command_test.dart index 4fa96bc1..cec43b6e 100644 --- a/packages/shorebird_ci/test/generate_command_test.dart +++ b/packages/shorebird_ci/test/generate_command_test.dart @@ -261,6 +261,110 @@ void main() { }, ); + test( + 'has_unit_tests gates the test + codecov steps', + () async { + // Two packages: one with a test/ dir, one without. The main + // workflow should pass has_unit_tests per package, and the + // reusable workflow should gate its test step on the input. + createPackage( + tempDir, + 'packages/with_tests', + 'with_tests', + addTestDir: true, + ); + createPackage(tempDir, 'packages/no_tests', 'no_tests'); + initGitRepo(tempDir); + + await runGenerate(tempDir, extra: ['--style', 'static']); + + final main = _readMain(tempDir); + expect( + main, + matches( + RegExp( + 'with_tests:[^#]*has_unit_tests: true', + dotAll: true, + ), + ), + ); + expect( + main, + matches( + RegExp( + 'no_tests:[^#]*has_unit_tests: false', + dotAll: true, + ), + ), + ); + + final reusable = File( + p.join( + tempDir.path, + '.github', + 'workflows', + '_shorebird_ci_dart.yaml', + ), + ).readAsStringSync(); + expect(reusable, contains('has_unit_tests:')); + expect(reusable, contains('if: inputs.has_unit_tests')); + }, + ); + + test( + 'YAML map keys default to the package name when unique', + () async { + // A repo where every package has a unique `name:` — the YAML + // keys are just the names, no path prefix. + createPackage(tempDir, 'packages/foo', 'foo'); + createPackage(tempDir, 'apps/bar', 'bar'); + initGitRepo(tempDir); + + await runGenerate(tempDir, extra: ['--style', 'static']); + final yaml = _readMain(tempDir); + + for (final slug in ['foo', 'bar']) { + expect(yaml, contains('$slug:')); + expect(yaml, contains('needs.changes.outputs.$slug')); + } + // No path-prefixed variants leak in. + expect(yaml, isNot(contains('packages_foo'))); + expect(yaml, isNot(contains('apps_bar'))); + }, + ); + + test( + 'YAML map keys fall back to _ on collision', + () async { + // Two packages w/ the same `name:` at different parent dirs: + // pub allows this, and shorebird_ci must disambiguate the YAML + // keys by walking up one path segment. `name: harness` under + // `apps/alpha/` becomes `alpha_harness`, under `apps/beta/` + // becomes `beta_harness`. A unique-named neighbor in the same + // repo keeps its plain name. + createPackage(tempDir, 'apps/alpha/harness', 'harness'); + createPackage(tempDir, 'apps/beta/harness', 'harness'); + createPackage(tempDir, 'packages/foo', 'foo'); + initGitRepo(tempDir); + + await runGenerate(tempDir, extra: ['--style', 'static']); + final yaml = _readMain(tempDir); + + for (final slug in ['alpha_harness', 'beta_harness', 'foo']) { + // Each slug appears as a dorny filter key, an outputs entry, + // and a per-package job key. + expect(yaml, contains('$slug:')); + expect(yaml, contains('needs.changes.outputs.$slug')); + } + // The actual package name still flows through as the + // `package_name:` input value (used for codecov flags). + expect(yaml, contains('package_name: harness')); + expect(yaml, contains('package_name: foo')); + // The unique package does not get prefixed. + expect(yaml, isNot(contains('packages_foo'))); + }, + ); + test('verify step comes before dorny filter', () async { createPackage(tempDir, 'packages/foo', 'foo'); initGitRepo(tempDir); diff --git a/packages/shorebird_ci/test/repository_analyzer_test.dart b/packages/shorebird_ci/test/repository_analyzer_test.dart index 6844210b..8e88e471 100644 --- a/packages/shorebird_ci/test/repository_analyzer_test.dart +++ b/packages/shorebird_ci/test/repository_analyzer_test.dart @@ -150,25 +150,21 @@ void main() { ); }); - test('throws on duplicate package names', () async { - // Two packages declaring `name: example` would silently collide - // when used as YAML map keys in the generated workflow. Fail - // loudly instead. + test('allows packages sharing a `name:` at different paths', () async { + // Two packages can legitimately share a `name:` (pub allows it + // when the packages are unrelated, e.g. test harnesses sitting + // next to multiple apps). The analyzer must surface both; the + // workflow generator disambiguates by path when it emits YAML + // map keys. createPackage(tempDir, 'a/example', 'example'); createPackage(tempDir, 'b/example', 'example'); initGitRepo(tempDir); final analyzer = RepositoryAnalyzer(); - expect( - () => analyzer.analyze(repositoryRoot: tempDir), - throwsA( - isA().having( - (e) => e.message, - 'message', - contains('Duplicate package names'), - ), - ), - ); + final repo = analyzer.analyze(repositoryRoot: tempDir); + + expect(repo.packages.length, 2); + expect(repo.packages.every((p) => p.name == 'example'), isTrue); }); test( diff --git a/packages/shorebird_ci/test/verify_command_test.dart b/packages/shorebird_ci/test/verify_command_test.dart index 416e5a69..5fea5ca0 100644 --- a/packages/shorebird_ci/test/verify_command_test.dart +++ b/packages/shorebird_ci/test/verify_command_test.dart @@ -125,6 +125,37 @@ jobs: expect(await runVerify(tempDir), 1); }); + test( + 'colliding package names match by slug, not by name', + () async { + // Two packages share `name: harness` at different parent dirs. + // `generate` emits the dorny filter keys as `alpha_harness` and + // `beta_harness`. Verify must compute the same slugs to + // recognize coverage; looking up by plain `pkg.name` + // ("harness") would miss both. + createPackage(tempDir, 'apps/alpha/harness', 'harness'); + createPackage(tempDir, 'apps/beta/harness', 'harness'); + _writeWorkflow(tempDir, 'ci.yaml', ''' +name: CI +on: [push] +jobs: + changes: + runs-on: ubuntu-latest + steps: + - uses: dorny/paths-filter@v3 + with: + filters: | + alpha_harness: + - apps/alpha/harness/** + beta_harness: + - apps/beta/harness/** +'''); + initGitRepo(tempDir); + + expect(await runVerify(tempDir), 0); + }, + ); + test('exposes a non-empty description', () { expect(VerifyCommand().description, isNotEmpty); });