Skip to content

Commit eaebdba

Browse files
authored
Add a CiYaml flag (unused in prod) that changes non-ToT post-submit filtering (#4730)
Towards flutter/flutter#169625. The next step would be adding this to `DynamicConfig`, and having `CiYamlFetcher` use it.
1 parent 5f244d9 commit eaebdba

2 files changed

Lines changed: 146 additions & 21 deletions

File tree

app_dart/lib/src/model/ci_yaml/ci_yaml.dart

Lines changed: 79 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -94,12 +94,12 @@ final class CiYamlSet {
9494
/// List of target names used to filter target from release candidate branches
9595
/// that were already removed from main.
9696
List<String>? totTargetNames({CiType type = CiType.any}) =>
97-
configs[type]!.totTargetNames;
97+
configs[type]!._totTargetNames;
9898

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

104104
/// Get an unfiltered list of all [targets] that are found in the ci.yaml file.
105105
List<Target> targets({CiType type = CiType.any}) => configs[type]!.targets;
@@ -117,6 +117,7 @@ class CiYaml {
117117
required this.branch,
118118
required this.config,
119119
required this.type,
120+
this.onlyUseTipOfTreeTargetsExistenceToFilterTargets = false,
120121
CiYaml? totConfig,
121122
bool validate = false,
122123
}) {
@@ -125,31 +126,88 @@ class CiYaml {
125126
}
126127
// Do not filter bringup targets. They are required for backward compatibility
127128
// with release candidate branches.
128-
final totTargets = totConfig?._targets ?? <Target>[];
129-
final totEnabledTargets = _filterEnabledTargets(totTargets);
130-
totTargetNames =
131-
totEnabledTargets.map((Target target) => target.name).toList();
132-
totPostsubmitTargetNames =
133-
totConfig?.postsubmitTargets
134-
.map((Target target) => target.name)
135-
.toList() ??
136-
<String>[];
129+
final totTargets = totConfig?._targets ?? const [];
130+
_totTargetNames = List.unmodifiable(totTargets.map((t) => t.name));
131+
if (onlyUseTipOfTreeTargetsExistenceToFilterTargets) {
132+
_totPostsubmitTargetNames = _totTargetNames;
133+
} else if (totConfig != null) {
134+
_totPostsubmitTargetNames = List.unmodifiable(
135+
totConfig.postsubmitTargets.map((t) => t.name),
136+
);
137+
} else {
138+
_totPostsubmitTargetNames = const [];
139+
}
137140
}
138141

142+
/// Whether to _only_ use the existence of a target at tip-of-tree to filter
143+
/// out targets on other branches.
144+
///
145+
/// By setting this flag to `true`, a tip-of-tree (default) branch's enabled
146+
/// branches cannot impact how a different branch's targets are enabled (or
147+
/// not), only the _existence_ of a tip-of-tree target is considered.
148+
///
149+
/// ## Details
150+
///
151+
/// As discovered in https://github.com/flutter/flutter/issues/169370, the
152+
/// default (or `master`/`main`) branch of a repository was required to have
153+
/// knowledge about all other possible branches, and if a new branch was
154+
/// created (but not defined in `enabledBranches: [ ...]`), the targets would
155+
/// automatically be filtered out.
156+
///
157+
/// Consider the following tip-of-tree definition of a `.ci.yaml`:
158+
/// ```yaml
159+
/// # flutter/flutter/master
160+
/// # //engine/src/flutter/.ci.yaml
161+
/// enabled_branches:
162+
/// - master
163+
/// - flutter-\d+\.\d+-candidate\.\d+
164+
///
165+
/// targets:
166+
/// - name: Mac host_engine
167+
/// properties:
168+
/// release_build: "true"
169+
/// - name: Mac host_engine_test
170+
/// ```
171+
///
172+
/// And the same file, tweaked for execution in branch `ios-experimental`:
173+
/// ```yaml
174+
/// # flutter/flutter/ios-experimental
175+
/// # //engine/src/flutter/.ci.yaml
176+
/// enabled_branches:
177+
/// - ios-experimental
178+
///
179+
/// targets:
180+
/// - name: Mac host_engine
181+
/// properties:
182+
/// release_build: "true"
183+
/// - name: Mac host_engine_test
184+
/// ```
185+
///
186+
/// If this flag is `false`, `Mac host_engine` does _not_ run on the
187+
/// `ios-experimental` branch's postsubmit, because it is configured to not
188+
/// run on `master`'s postsubmit (it is a `release_build`).
189+
///
190+
/// This is confusing behavior, where experimental branches would need to
191+
/// modify the tip-of-tree `.ci.yaml` in order to allow targets to be executed
192+
/// on their branch, so this flag exists to change that behavior.
193+
final bool onlyUseTipOfTreeTargetsExistenceToFilterTargets;
194+
139195
final CiType type;
140196

141197
/// Whether [slug] is [Config.flutterSlug], the fusion `flutter/flutter` repo.
142198
///
143199
/// If `true`, multiple `.ci.yaml` files are available. See [CiType].
144200
bool get isFusion => slug == Config.flutterSlug;
145201

146-
/// List of target names used to filter target from release candidate branches
147-
/// that were already removed from main.
148-
List<String>? totTargetNames;
202+
/// Target names that exist at tip-of-tree.
203+
///
204+
/// This list is unmodifiable.
205+
late final List<String> _totTargetNames;
149206

150-
/// List of postsubmit target names used to filter target from release candidate branches
151-
/// that were already removed from main.
152-
List<String>? totPostsubmitTargetNames;
207+
/// Target names that execute on postsubmit at tip-of-tree.
208+
///
209+
/// This list is unmodifiable.
210+
late final List<String> _totPostsubmitTargetNames;
153211

154212
/// The underlying protobuf that contains the raw data from .ci.yaml.
155213
pb.SchedulerConfig config;
@@ -171,11 +229,11 @@ class CiYaml {
171229
throw BranchNotEnabledForThisCiYamlException(branch: branch);
172230
}
173231
// Filter targets removed from main.
174-
if (totTargetNames!.isNotEmpty) {
232+
if (_totTargetNames.isNotEmpty) {
175233
enabledTargets = _filterOutdatedTargets(
176234
slug,
177235
enabledTargets,
178-
totTargetNames!,
236+
_totTargetNames,
179237
);
180238
}
181239
return enabledTargets;
@@ -189,11 +247,11 @@ class CiYaml {
189247

190248
var enabledTargets = _filterEnabledTargets(postsubmitTargets);
191249
// Filter targets removed from main.
192-
if (totPostsubmitTargetNames!.isNotEmpty) {
250+
if (_totPostsubmitTargetNames.isNotEmpty) {
193251
enabledTargets = _filterOutdatedTargets(
194252
slug,
195253
enabledTargets,
196-
totPostsubmitTargetNames!,
254+
_totPostsubmitTargetNames,
197255
);
198256
}
199257
// filter if release_build true if current branch is a release candidate branch, or a fusion tree.

app_dart/test/model/ci_yaml/ci_yaml_test.dart

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import 'package:cocoon_service/src/model/ci_yaml/target.dart';
99
import 'package:cocoon_service/src/service/config.dart';
1010
import 'package:test/test.dart';
1111

12+
import '../../src/model/ci_yaml_matcher.dart';
1213
import '../../src/service/fake_scheduler.dart';
1314

1415
void main() {
@@ -106,6 +107,72 @@ void main() {
106107
expect(targets.map((target) => target.name), containsAll(['Linux A']));
107108
});
108109

110+
// TODO(matanlurey): Remove after legacy behavior removed.
111+
// See https://github.com/flutter/flutter/issues/169370.
112+
test('filters targets not enabled at ToT for the current branch', () {
113+
final ciYaml = CiYaml(
114+
slug: Config.flutterSlug,
115+
branch: 'ios-experimental',
116+
config: pb.SchedulerConfig(
117+
enabledBranches: ['ios-experimental'],
118+
targets: [pb.Target(name: 'Linux host_engine')],
119+
),
120+
totConfig: CiYaml(
121+
slug: Config.flutterSlug,
122+
branch: 'master',
123+
config: pb.SchedulerConfig(
124+
enabledBranches: ['master'],
125+
targets: [
126+
pb.Target(
127+
name: 'Linux host_engine',
128+
properties: {'release_build': 'true'},
129+
),
130+
// By adding a postsubmit test, it triggers this weird edge case.
131+
pb.Target(name: 'Linux host_engine_tests'),
132+
],
133+
),
134+
type: CiType.any,
135+
),
136+
type: CiType.any,
137+
);
138+
expect(
139+
ciYaml.postsubmitTargets,
140+
isEmpty,
141+
reason: 'At ToT, Linux host_engine does not run in postsubmit.',
142+
);
143+
});
144+
145+
// Regression test for https://github.com/flutter/flutter/issues/169370.
146+
test('targets not enabled at ToT does not impact current branch', () {
147+
final ciYaml = CiYaml(
148+
slug: Config.flutterSlug,
149+
branch: 'ios-experimental',
150+
onlyUseTipOfTreeTargetsExistenceToFilterTargets: true,
151+
config: pb.SchedulerConfig(
152+
enabledBranches: ['ios-experimental'],
153+
targets: [pb.Target(name: 'Linux host_engine')],
154+
),
155+
totConfig: CiYaml(
156+
slug: Config.flutterSlug,
157+
branch: 'master',
158+
config: pb.SchedulerConfig(
159+
enabledBranches: ['master'],
160+
targets: [
161+
pb.Target(
162+
name: 'Linux host_engine',
163+
properties: {'release_build': 'true'},
164+
),
165+
// By adding a postsubmit test, it triggers this weird edge case.
166+
pb.Target(name: 'Linux host_engine_tests'),
167+
],
168+
),
169+
type: CiType.any,
170+
),
171+
type: CiType.any,
172+
);
173+
expect(ciYaml.postsubmitTargets, [isTarget.hasName('Linux host_engine')]);
174+
});
175+
109176
group('validations and filters.', () {
110177
final totCIYaml = CiYaml(
111178
type: CiType.any,

0 commit comments

Comments
 (0)