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

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
Prev Previous commit
Add TODO where appropriate based on feedback.
  • Loading branch information
@matanlurey
matanlurey committed Dec 26, 2024
commit 5ea0d11dd5a0f55a3fb69302e4872251c8f7dc35
9 changes: 9 additions & 0 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 @@ -427,11 +427,15 @@ class CiYaml {
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.
// TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser?
// See https://github.com/flutter/flutter/issues/160874.
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

// TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser?
// See https://github.com/flutter/flutter/issues/160874.
if (slug == Config.engineSlug && !target.runIf.contains('DEPS')) {
exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf');
}
Expand All @@ -447,6 +451,9 @@ class CiYaml {
// 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.
//
// TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser?
// See https://github.com/flutter/flutter/issues/160874.
final ciYamlPath = switch (type) {
CiType.fusionEngine => 'engine/src/flutter/.ci.yaml',
_ => '.ci.yaml',
Expand All @@ -456,6 +463,8 @@ class CiYaml {
}

// 2. Every target must depend on DEPS.
// TODO(matanlurey): Can this be inferred instead in the .ci.yaml parser?
// See https://github.com/flutter/flutter/issues/160874.
if (!target.runIf.contains('DEPS')) {
exceptions.add('ERROR: ${target.name} is missing `DEPS` in runIf');
}
Expand Down