Skip to content

Fix .ci.yaml validation for Fusion (the monorepo) #4137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 6 commits into from
Dec 26, 2024
Merged

Fix .ci.yaml validation for Fusion (the monorepo) #4137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1e4135e
Fix and write unit tests for .ci.yaml runIf validation.
matanlurey Dec 23, 2024
fb36ba5
Format.
matanlurey Dec 23, 2024
d470bd1
Reformat.
matanlurey Dec 23, 2024
44df905
No idea man.
matanlurey Dec 23, 2024
0da4e33
Update .ci.yaml example in scheduler_test.
matanlurey Dec 23, 2024
5ea0d11
Add TODO where appropriate based on feedback.
matanlurey Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix and write unit tests for .ci.yaml runIf validation.
  • Loading branch information
@matanlurey
matanlurey committed Dec 23, 2024
commit 1e4135e88ce25dd6898d30c791c31463b278ca20
196 changes: 135 additions & 61 deletions app_dart/lib/src/model/ci_yaml/ci_yaml.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,12 @@ class CiYamlSet {
final String branch;

/// Gets all [Target] that run on presubmit for this config.
List<Target> presubmitTargets({CiType type = CiType.any}) => configs[type]!.presubmitTargets;
List<Target> presubmitTargets({CiType type = CiType.any}) =>
configs[type]!.presubmitTargets;

/// Gets all [Target] that run on postsubmit for this config.
List<Target> postsubmitTargets({CiType type = CiType.any}) => configs[type]!.postsubmitTargets;
List<Target> postsubmitTargets({CiType type = CiType.any}) =>
configs[type]!.postsubmitTargets;

/// Gets the first [Target] matching [builderName] or null.
Target? getFirstPostsubmitTarget(
Expand All @@ -90,14 +92,17 @@ class CiYamlSet {

/// List of target names used to filter target from release candidate branches
/// that were already removed from main.
List<String>? totTargetNames({CiType type = CiType.any}) => configs[type]!.totTargetNames;
List<String>? totTargetNames({CiType type = CiType.any}) =>
configs[type]!.totTargetNames;

/// List of postsubmit target names used to filter target from release candidate branches
/// that were already removed from main.
List<String>? totPostsubmitTargetNames({CiType type = CiType.any}) => configs[type]!.totPostsubmitTargetNames;
List<String>? totPostsubmitTargetNames({CiType type = CiType.any}) =>
configs[type]!.totPostsubmitTargetNames;

/// Filters post submit targets to remove targets we do not want backfilled.
List<Target> backfillTargets({CiType type = CiType.any}) => configs[type]!.backfillTargets;
List<Target> backfillTargets({CiType type = CiType.any}) =>
configs[type]!.backfillTargets;

/// Filters targets that were removed from main. [slug] is the gihub
/// slug for branch under test, [targets] is the list of targets from
Expand Down Expand Up @@ -149,9 +154,12 @@ class CiYaml {
// with release candidate branches.
final Iterable<Target> totTargets = totConfig?._targets ?? <Target>[];
final List<Target> totEnabledTargets = _filterEnabledTargets(totTargets);
totTargetNames = totEnabledTargets.map((Target target) => target.value.name).toList();
totPostsubmitTargetNames =
totConfig?.postsubmitTargets.map((Target target) => target.value.name).toList() ?? <String>[];
totTargetNames =
totEnabledTargets.map((Target target) => target.value.name).toList();
totPostsubmitTargetNames = totConfig?.postsubmitTargets
.map((Target target) => target.value.name)
.toList() ??
<String>[];
}

final CiType type;
Expand All @@ -177,28 +185,32 @@ class CiYaml {

/// Gets all [Target] that run on presubmit for this config.
List<Target> get presubmitTargets {
final Iterable<Target> presubmitTargets =
_targets.where((Target target) => target.value.presubmit && !target.value.bringup);
final Iterable<Target> presubmitTargets = _targets.where(
(Target target) => target.value.presubmit && !target.value.bringup);
List<Target> enabledTargets = _filterEnabledTargets(presubmitTargets);

if (enabledTargets.isEmpty) {
throw Exception('$branch is not enabled for this .ci.yaml.\nAdd it to run tests against this PR.');
throw Exception(
'$branch is not enabled for this .ci.yaml.\nAdd it to run tests against this PR.');
}
// Filter targets removed from main.
if (totTargetNames!.isNotEmpty) {
enabledTargets = filterOutdatedTargets(slug, enabledTargets, totTargetNames);
enabledTargets =
filterOutdatedTargets(slug, enabledTargets, totTargetNames);
}
return enabledTargets;
}

/// Gets all [Target] that run on postsubmit for this config.
List<Target> get postsubmitTargets {
final Iterable<Target> postsubmitTargets = _targets.where((Target target) => target.value.postsubmit);
final Iterable<Target> postsubmitTargets =
_targets.where((Target target) => target.value.postsubmit);

List<Target> enabledTargets = _filterEnabledTargets(postsubmitTargets);
// Filter targets removed from main.
if (totPostsubmitTargetNames!.isNotEmpty) {
enabledTargets = filterOutdatedTargets(slug, enabledTargets, totPostsubmitTargetNames);
enabledTargets =
filterOutdatedTargets(slug, enabledTargets, totPostsubmitTargetNames);
}
// filter if release_build true if current branch is a release candidate branch, or a fusion tree.
enabledTargets = _selectTargetsForBranch(enabledTargets);
Expand All @@ -207,15 +219,17 @@ class CiYaml {

/// Gets the first [Target] matching [builderName] or null.
Target? getFirstPostsubmitTarget(String builderName) {
return postsubmitTargets.singleWhereOrNull((Target target) => target.value.name == builderName);
return postsubmitTargets
.singleWhereOrNull((Target target) => target.value.name == builderName);
}

/// Filters post submit targets to remove targets we do not want backfilled.
List<Target> get backfillTargets {
final List<Target> filteredTargets = <Target>[];
for (Target target in postsubmitTargets) {
final Map<String, Object> properties = target.getProperties();
if (!properties.containsKey('backfill') || properties['backfill'] as bool) {
if (!properties.containsKey('backfill') ||
properties['backfill'] as bool) {
filteredTargets.add(target);
}
}
Expand All @@ -231,7 +245,10 @@ class CiYaml {
// For release branches we don't want to run release targets or bringup
// targets because they are built outside of Cocoon. This applies in both
// fusion and non-fusion repos.
return [...targets.where((target) => !target.isReleaseBuildTarget && !target.isBringupTarget)];
return [
...targets.where(
(target) => !target.isReleaseBuildTarget && !target.isBringupTarget)
];
} else {
// For non-release branches we also want to include bringup targets.
// However, there's a difference between fusion and non-fusion repos.
Expand All @@ -256,7 +273,8 @@ class CiYaml {
return targets
.where(
(Target target) =>
(target.value.enabledBranches.isNotEmpty && !target.value.enabledBranches.contains(defaultBranch)) ||
(target.value.enabledBranches.isNotEmpty &&
!target.value.enabledBranches.contains(defaultBranch)) ||
totTargetNames!.contains(target.value.name),
)
.toList();
Expand All @@ -268,10 +286,13 @@ class CiYaml {
/// This shouldn't be confused for targets that have the property named dependency, which is used by the
/// flutter_deps recipe module on LUCI.
List<Target> getInitialTargets(List<Target> targets) {
Iterable<Target> initialTargets = targets.where((Target target) => target.value.dependencies.isEmpty).toList();
Iterable<Target> initialTargets = targets
.where((Target target) => target.value.dependencies.isEmpty)
.toList();
if (branch != Config.defaultBranch(slug)) {
// Filter out bringup targets for release branches
initialTargets = initialTargets.where((Target target) => !target.value.bringup);
initialTargets =
initialTargets.where((Target target) => !target.value.bringup);
}

return initialTargets.toList();
Expand All @@ -297,27 +318,31 @@ class CiYaml {
final List<Target> filteredTargets = <Target>[];

final ghMqBranch = tryParseGitHubMergeQueueBranch(branch);
final realBranch = ghMqBranch == notGitHubMergeQueueBranch ? branch : ghMqBranch.branch;
final realBranch =
ghMqBranch == notGitHubMergeQueueBranch ? branch : ghMqBranch.branch;

// 1. Add targets with local definition
final Iterable<Target> overrideBranchTargets =
targets.where((Target target) => target.value.enabledBranches.isNotEmpty);
final Iterable<Target> enabledTargets = overrideBranchTargets
.where((Target target) => enabledBranchesMatchesCurrentBranch(target.value.enabledBranches, realBranch));
final Iterable<Target> overrideBranchTargets = targets
.where((Target target) => target.value.enabledBranches.isNotEmpty);
final Iterable<Target> enabledTargets = overrideBranchTargets.where(
(Target target) => enabledBranchesMatchesCurrentBranch(
target.value.enabledBranches, realBranch));
filteredTargets.addAll(enabledTargets);

// 2. Add targets with global definition (this is the majority of targets)
if (enabledBranchesMatchesCurrentBranch(config.enabledBranches, realBranch)) {
final Iterable<Target> defaultBranchTargets =
targets.where((Target target) => target.value.enabledBranches.isEmpty);
if (enabledBranchesMatchesCurrentBranch(
config.enabledBranches, realBranch)) {
final Iterable<Target> defaultBranchTargets = targets
.where((Target target) => target.value.enabledBranches.isEmpty);
filteredTargets.addAll(defaultBranchTargets);
}

return filteredTargets;
}

/// Whether any of the possible [RegExp] in [enabledBranches] match [branch].
static bool enabledBranchesMatchesCurrentBranch(List<String> enabledBranches, String branch) {
static bool enabledBranchesMatchesCurrentBranch(
List<String> enabledBranches, String branch) {
final List<String> regexes = <String>[];
for (String enabledBranch in enabledBranches) {
// Prefix with start of line and suffix with end of line
Expand All @@ -343,17 +368,21 @@ class CiYaml {
/// 5. [pb.Target] should not depend on self
/// 6. [pb.Target] cannot have more than 1 dependency
/// 7. [pb.Target] should depend on target that already exist in depedency graph, and already recorded in map [targetGraph]
/// 8. [pb.Target] has an empty runIf or the runIf includes `.ci.yaml` and `DEPS if on the engine repo.
void _validate(pb.SchedulerConfig schedulerConfig, String branch, {pb.SchedulerConfig? totSchedulerConfig}) {
/// 8. [pb.Target] has an empty runIf or the runIf includes `.ci.yaml` `DEPS`, and `engine/**` if validating the framework.
void _validate(pb.SchedulerConfig schedulerConfig, String branch,
{pb.SchedulerConfig? totSchedulerConfig}) {
if (schedulerConfig.targets.isEmpty) {
throw const FormatException('Scheduler config must have at least 1 target');
throw const FormatException(
'Scheduler config must have at least 1 target');
}

if (schedulerConfig.enabledBranches.isEmpty) {
throw const FormatException('Scheduler config must have at least 1 enabled branch');
throw const FormatException(
'Scheduler config must have at least 1 enabled branch');
}

final Map<String, List<pb.Target>> targetGraph = <String, List<pb.Target>>{};
final Map<String, List<pb.Target>> targetGraph =
<String, List<pb.Target>>{};
final List<String> exceptions = <String>[];
final Set<String> totTargets = <String>{};
if (totSchedulerConfig != null) {
Expand All @@ -369,7 +398,9 @@ class CiYaml {
} else {
// a new build without "bringup: true"
// link to wiki - https://github.com/flutter/flutter/blob/master/docs/infra/Reducing-Test-Flakiness.md#adding-a-new-devicelab-test
if (totTargets.isNotEmpty && !totTargets.contains(target.name) && target.bringup != true) {
if (totTargets.isNotEmpty &&
!totTargets.contains(target.name) &&
target.bringup != true) {
exceptions.add(
'ERROR: ${target.name} is a new builder added. it needs to be marked bringup: true\nIf ci.yaml wasn\'t changed, try `git fetch upstream && git merge upstream/master`',
);
Expand All @@ -379,41 +410,34 @@ class CiYaml {
// Add edges
if (target.dependencies.isNotEmpty) {
if (target.dependencies.length != 1) {
exceptions
.add('ERROR: ${target.name} has multiple dependencies which is not supported. Use only one dependency');
exceptions.add(
'ERROR: ${target.name} has multiple dependencies which is not supported. Use only one dependency');
} else {
if (target.dependencies.first == target.name) {
exceptions.add('ERROR: ${target.name} cannot depend on itself');
} else if (targetGraph.containsKey(target.dependencies.first)) {
targetGraph[target.dependencies.first]!.add(target);
} else {
exceptions.add('ERROR: ${target.name} depends on ${target.dependencies.first} which does not exist');
exceptions.add(
'ERROR: ${target.name} depends on ${target.dependencies.first} which does not exist');
}
}
}

// Verify runIf includes foundational files.
if (target.runIf.isNotEmpty) {
if (isFusion && type == CiType.fusionEngine) {
// Look in different locations if fusion && engine ci.yaml
if (!target.runIf.contains('engine/src/flutter/.ci.yaml')) {
exceptions.add(
'ERROR: ${target.name} is missing `engine/src/flutter/.ci.yaml` in runIf',
);
}
if (!target.runIf.contains('DEPS')) {
exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf');
}
if (isFusion) {
_verifyTargetFusionRepo(target, exceptions);
} else {
// not fusion or not engine in fusion.
if (!target.runIf.contains('.ci.yaml')) {
exceptions.add('ERROR: ${target.name} is missing `.ci.yaml` in runIf');
}
if (slug == Config.engineSlug && !target.runIf.contains('DEPS')) {
exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf');
}
_verifyTargetClassicRepo(target, exceptions);
}
}

// Verify runIfNot is never used, as it is a very confusing feature.
// https://github.com/flutter/flutter/issues/147182
if (target.runIfNot.isNotEmpty) {
exceptions.add('ERROR: ${target.name}: Do not use runIfNot.');
}
}

/// Check the dependencies for the current target if it is viable and to
Expand All @@ -429,9 +453,56 @@ class CiYaml {
_checkExceptions(exceptions);
}

void _verifyTargetClassicRepo(pb.Target target, List<String> exceptions) {
// These serve mostly as documentation.
assert(!isFusion, 'Expected to be called in a non-monorepo');
assert(target.runIf.isNotEmpty, 'Expected to be called when non-empty');

// 1. Every target must depend on .ci.yaml at the root of the repo.
if (!target.runIf.contains('.ci.yaml')) {
exceptions.add('ERROR: ${target.name} is missing `.ci.yaml` in runIf');
}

// 2. The engine repo must additionally depend on DEPS.
Copy link
Contributor

@yjbanov yjbanov Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please leave a TODO with a link to an issue reminding us to clean this up once we're ready to archive the engine repo?

Copy link
Contributor Author

@matanlurey matanlurey Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed as flutter/flutter#160874

if (slug == Config.engineSlug && !target.runIf.contains('DEPS')) {
exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf');
}
}

void _verifyTargetFusionRepo(pb.Target target, List<String> exceptions) {
// These serve mostly as documentation.
assert(isFusion, 'Expected to be called in a fusion monorepo');
assert(target.runIf.isNotEmpty, 'Expected to be called when non-empty');
assert(slug == Config.flutterSlug, 'Expected to be in the combined repo');

// 1. Every target must depend on .ci.yaml.
// The path depends on whether the framework or engine are being validated;
// while both belong in the same (mono)repo, they have separate .ci.yaml
// files located in different paths.
final ciYamlPath = switch (type) {
CiType.fusionEngine => 'engine/src/flutter/.ci.yaml',
_ => '.ci.yaml',
};
Copy link
Contributor

@yjbanov yjbanov Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these be added automatically by the .ci.yaml parser (or by the logic that evaluates runIf conditions)? This way we don't have to specify anything in the yaml files. Seems like they add a bunch of noise. Totally fine we want to take this one step at a time though.

Copy link
Contributor Author

@matanlurey matanlurey Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed as flutter/flutter#160874

if (!target.runIf.contains(ciYamlPath)) {
exceptions.add('ERROR: ${target.name} is missing `$ciYamlPath` in runIf');
}

// 2. Every target must depend on DEPS.
if (!target.runIf.contains('DEPS')) {
exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf');
}

// 3. The framework tree must depend on engine/**.
final isFrameworkCiYaml = type != CiType.fusionEngine;
if (isFrameworkCiYaml && !target.runIf.contains('engine/**')) {
exceptions.add('ERROR: ${target.name} is missing `engine/**` in runIf');
}
}

void _checkExceptions(List<String> exceptions) {
if (exceptions.isNotEmpty) {
final String fullException = exceptions.reduce((String exception, _) => '$exception\n');
final String fullException =
exceptions.reduce((String exception, _) => '$exception\n');
throw FormatException(fullException);
}
}
Expand All @@ -449,22 +520,25 @@ class DependencyValidator {
final List<String> exceptions = <String>[];

/// Decoded will contain a list of maps for the dependencies found.
final List<dynamic> decoded = json.decode(dependencyJsonString) as List<dynamic>;
final List<dynamic> decoded =
json.decode(dependencyJsonString) as List<dynamic>;

for (Map<String, dynamic> depMap in decoded) {
if (!depMap.containsKey('version')) {
exceptions.add('ERROR: dependency ${depMap['dependency']} must have a version.');
exceptions.add(
'ERROR: dependency ${depMap['dependency']} must have a version.');
} else {
final String version = depMap['version'] as String;
if (version.isEmpty || version == 'latest') {
exceptions
.add('ERROR: dependency ${depMap['dependency']} must have a non empty, non "latest" version supplied.');
exceptions.add(
'ERROR: dependency ${depMap['dependency']} must have a non empty, non "latest" version supplied.');
}
}
}

if (exceptions.isNotEmpty) {
final String fullException = exceptions.reduce((String exception, _) => '$exception\n');
final String fullException =
exceptions.reduce((String exception, _) => '$exception\n');
throw FormatException(fullException);
}
}
Expand Down
Loading