Skip to content

Remove completed flag migrations for CiYaml. #4762

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
merged 3 commits into from
Jun 4, 2025
Merged

Remove completed flag migrations for CiYaml. #4762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
73b88b0
Remove completed flag migrations for CiYaml.
matanlurey Jun 4, 2025
6296e4c
++
matanlurey Jun 4, 2025
fc4452e
++
matanlurey Jun 4, 2025
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
4 changes: 4 additions & 0 deletions app_dart/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,12 @@
backfillerCommitLimit: 50

ciYaml:
# ----------------------------------------------------------------------------
# TODO(matanlurey): These are inert in the latest builds and can be removed
# a few days after https://github.com/flutter/cocoon/pull/4762 is merged.
Comment on lines +10 to +11
Copy link
Member

Choose a reason for hiding this comment

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

14 days if the "old images are removed after 14 days" rule is being followed.

onlyUseTipOfTreeTargetsExistenceToFilterTargets: true
targetEnabledBranchesOverridesTipOfTreeTargetExistence: false
# ----------------------------------------------------------------------------

contentAwareHashing:
# This will cause PRs that enter the merge queue to wait on CAH hashing
Expand Down
20 changes: 2 additions & 18 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 @@ -131,18 +131,11 @@ class CiYaml {
// with release candidate branches.
final totTargets = totConfig?._targets ?? const [];
_totTargetNames = List.unmodifiable(totTargets.map((t) => t.name));
if (_flags.onlyUseTipOfTreeTargetsExistenceToFilterTargets) {
_totPostsubmitTargetNames = _totTargetNames;
} else if (totConfig != null) {
_totPostsubmitTargetNames = List.unmodifiable(
totConfig.postsubmitTargets.map((t) => t.name),
);
} else {
_totPostsubmitTargetNames = const [];
}
_totPostsubmitTargetNames = _totTargetNames;
}

/// Flags related to resolving `.ci.yaml`.
// ignore: unused_field
final CiYamlFlags _flags;

/// The type of `.ci.yaml` `this` represents.
Expand Down Expand Up @@ -255,16 +248,7 @@ class CiYaml {
Iterable<Target> targets,
Iterable<String> totTargetNames,
) {
final defaultBranch = Config.defaultBranch(slug);
return targets.where((target) {
final forcedEnabledBranches = target.enabledBranches;
if (_flags.targetEnabledBranchesOverridesTipOfTreeTargetExistence &&
forcedEnabledBranches.isNotEmpty &&
!forcedEnabledBranches.contains(defaultBranch)) {
return true;
}

// Otherwise, the target must exist in Tip-of-Tree.
return totTargetNames.contains(target.name);
}).toList();
}
Expand Down
123 changes: 4 additions & 119 deletions app_dart/lib/src/service/flags/ci_yaml_flags.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,130 +12,15 @@ part 'ci_yaml_flags.g.dart';
@immutable
final class CiYamlFlags {
/// Default configuration for [CiYamlFlags] flags.
static const defaultInstance = CiYamlFlags._(
onlyUseTipOfTreeTargetsExistenceToFilterTargets: false,
targetEnabledBranchesOverridesTipOfTreeTargetExistence: true,
);
static const defaultInstance = CiYamlFlags._();

/// Whether to _only_ use the existence of a target at tip-of-tree to filter
/// out targets on other branches.
///
/// By setting this flag to `true`, a tip-of-tree (default) branch's enabled
/// branches cannot impact how a different branch's targets are enabled (or
/// not), only the _existence_ of a tip-of-tree target is considered.
///
/// ## Details
///
/// As discovered in https://github.com/flutter/flutter/issues/169370, the
/// default (or `master`/`main`) branch of a repository was required to have
/// knowledge about all other possible branches, and if a new branch was
/// created (but not defined in `enabledBranches: [ ...]`), the targets would
/// automatically be filtered out.
///
/// Consider the following tip-of-tree definition of a `.ci.yaml`:
/// ```yaml
/// # flutter/flutter/master
/// # //engine/src/flutter/.ci.yaml
/// enabled_branches:
/// - master
/// - flutter-\d+\.\d+-candidate\.\d+
///
/// targets:
/// - name: Mac host_engine
/// properties:
/// release_build: "true"
/// - name: Mac host_engine_test
/// ```
///
/// And the same file, tweaked for execution in branch `ios-experimental`:
/// ```yaml
/// # flutter/flutter/ios-experimental
/// # //engine/src/flutter/.ci.yaml
/// enabled_branches:
/// - ios-experimental
///
/// targets:
/// - name: Mac host_engine
/// properties:
/// release_build: "true"
/// - name: Mac host_engine_test
/// ```
///
/// If this flag is `false`, `Mac host_engine` does _not_ run on the
/// `ios-experimental` branch's postsubmit, because it is configured to not
/// run on `master`'s postsubmit (it is a `release_build`).
///
/// This is confusing behavior, where experimental branches would need to
/// modify the tip-of-tree `.ci.yaml` in order to allow targets to be executed
/// on their branch, so this flag exists to change that behavior.
@JsonKey()
final bool onlyUseTipOfTreeTargetsExistenceToFilterTargets;

/// Whether a target-specific `enabled_branches: [...]` overrides existence of
/// the target at tip-of-tree.
///
/// By setting this flag to `false`, the use of `enabled_branches: [ ... ]`
/// does _not_ take precedence on the target itself not existing at
/// tip-of-tree.
///
/// ## Details
///
/// Consider the following tip-of-tree definition of a `.ci.yaml`:
/// ```yaml
/// # flutter/flutter/master
/// # //engine/src/flutter/.ci.yaml
/// enabled_branches:
/// - master
///
/// targets:
/// - name: Mac foo
/// ```
///
/// And the same file, i.e. in a release branch:
/// ```yaml
/// # flutter/flutter/flutter-1.23-candidate.0
/// # //engine/src/flutter/.ci.yaml
/// enabled_branches:
/// - flutter-1.23-candidate.0
///
/// targets:
/// - name: Mac foo
/// - name: Mac bar
/// enabled_branches:
/// - flutter-1.23-candidate.0
/// ```
///
/// If this flag is `true`, `Mac bar` is considered to be runnable, even
/// though the target no longer exists at tip-of-tree (`master`).
///
/// It's not clear this feature ever even worked, because if it doesn't exist
/// at tip-of-tree, how could it even be executing? It's possible it's an
/// artifact of an older CI system that did not need targets to exist at
/// tip-of-tree (i.e. Cirrus).
@JsonKey()
final bool targetEnabledBranchesOverridesTipOfTreeTargetExistence;

const CiYamlFlags._({
required this.onlyUseTipOfTreeTargetsExistenceToFilterTargets, //
required this.targetEnabledBranchesOverridesTipOfTreeTargetExistence,
});
const CiYamlFlags._();

/// Creates [CiYamlFlags] flags from the provided fields.
///
/// Any omitted fields default to the values in [defaultInstance].
factory CiYamlFlags({
bool? onlyUseTipOfTreeTargetsExistenceToFilterTargets, //
bool? targetEnabledBranchesOverridesTipOfTreeTargetExistence,
}) {
return CiYamlFlags._(
onlyUseTipOfTreeTargetsExistenceToFilterTargets:
onlyUseTipOfTreeTargetsExistenceToFilterTargets ??
defaultInstance.onlyUseTipOfTreeTargetsExistenceToFilterTargets,
targetEnabledBranchesOverridesTipOfTreeTargetExistence:
targetEnabledBranchesOverridesTipOfTreeTargetExistence ??
defaultInstance
.targetEnabledBranchesOverridesTipOfTreeTargetExistence,
);
factory CiYamlFlags() {
return const CiYamlFlags._();
}

/// Creates [ContentAwareHashing] flags from a [json] object.
Expand Down
14 changes: 2 additions & 12 deletions app_dart/lib/src/service/flags/ci_yaml_flags.g.dart
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 1 addition & 38 deletions app_dart/test/model/ci_yaml/ci_yaml_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,49 +108,12 @@ void main() {
expect(targets.map((target) => target.name), containsAll(['Linux A']));
});

// TODO(matanlurey): Remove after legacy behavior removed.
// See https://github.com/flutter/flutter/issues/169370.
test('filters targets not enabled at ToT for the current branch', () {
final ciYaml = CiYaml(
slug: Config.flutterSlug,
branch: 'ios-experimental',
config: pb.SchedulerConfig(
enabledBranches: ['ios-experimental'],
targets: [pb.Target(name: 'Linux host_engine')],
),
totConfig: CiYaml(
slug: Config.flutterSlug,
branch: 'master',
config: pb.SchedulerConfig(
enabledBranches: ['master'],
targets: [
pb.Target(
name: 'Linux host_engine',
properties: {'release_build': 'true'},
),
// By adding a postsubmit test, it triggers this weird edge case.
pb.Target(name: 'Linux host_engine_tests'),
],
),
type: CiType.any,
),
type: CiType.any,
);
expect(
ciYaml.postsubmitTargets,
isEmpty,
reason: 'At ToT, Linux host_engine does not run in postsubmit.',
);
});

// Regression test for https://github.com/flutter/flutter/issues/169370.
test('targets not enabled at ToT does not impact current branch', () {
final ciYaml = CiYaml(
slug: Config.flutterSlug,
branch: 'ios-experimental',
flags: CiYamlFlags(
onlyUseTipOfTreeTargetsExistenceToFilterTargets: true,
),
flags: CiYamlFlags(),
config: pb.SchedulerConfig(
enabledBranches: ['ios-experimental'],
targets: [pb.Target(name: 'Linux host_engine')],
Expand Down