Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion app_dart/bin/gae_server.dart
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@ Future<void> main() async {
/// Github checks api service used to provide luci test execution status on the Github UI.
final githubChecksService = GithubChecksService(config);

final ciYamlFetcher = CiYamlFetcher(cache: cache, firestore: firestore);
final ciYamlFetcher = CiYamlFetcher(
config: config,
cache: cache,
firestore: firestore,
);

/// Cocoon scheduler service to manage validating commits in presubmit and postsubmit.
final scheduler = Scheduler(
Expand Down
2 changes: 2 additions & 0 deletions app_dart/config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

backfillerCommitLimit: 50

onlyUseTipOfTreeTargetsExistenceToFilterTargets: true
Comment on lines 7 to +8
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do this in its own PR so it can be reverted cleanly if needed.


contentAwareHashing:
# This will cause PRs that enter the merge queue to wait on CAH hashing
# to happen before scheduling builds.
Expand Down
7 changes: 6 additions & 1 deletion app_dart/lib/src/model/ci_yaml/ci_yaml.dart
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ final class CiYamlSet {
required this.slug,
required this.branch,
required Map<CiType, pb.SchedulerConfig> yamls,
bool onlyUseTipOfTreeTargetsExistenceToFilterTargets =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we make this required without reaching out to Config.kOnlyUseTipOfTreeTargetsExistenceToFilterTargets?

Config.kOnlyUseTipOfTreeTargetsExistenceToFilterTargets,
CiYamlSet? totConfig,
bool validate = false,
}) {
Expand All @@ -50,6 +52,8 @@ final class CiYamlSet {
type: type,
validate: validate,
totConfig: totConfig?.configs[type],
onlyUseTipOfTreeTargetsExistenceToFilterTargets:
onlyUseTipOfTreeTargetsExistenceToFilterTargets,
);
}
}
Expand Down Expand Up @@ -117,7 +121,8 @@ class CiYaml {
required this.branch,
required this.config,
required this.type,
this.onlyUseTipOfTreeTargetsExistenceToFilterTargets = false,
this.onlyUseTipOfTreeTargetsExistenceToFilterTargets =
Config.kOnlyUseTipOfTreeTargetsExistenceToFilterTargets,
CiYaml? totConfig,
bool validate = false,
}) {
Expand Down
23 changes: 21 additions & 2 deletions app_dart/lib/src/service/config.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

/// @docImport '../model/ci_yaml/ci_yaml.dart';
library;

import 'dart:convert';
import 'dart:math' show Random;
import 'dart:typed_data';
Expand Down Expand Up @@ -31,12 +34,18 @@ const String kDefaultBranchName = 'master';

interface class Config {
/// Creates and returns a [Config] instance.
Config(this._cache, this._secrets, {required DynamicConfig dynamicConfig})
: _dynamicConfig = dynamicConfig;
Config(
this._cache, //
this._secrets, {
required DynamicConfig dynamicConfig,
}) : _dynamicConfig = dynamicConfig;

/// Access dynamically configured flags.
DynamicConfig get flags => _dynamicConfig;

/// See [CiYaml.onlyUseTipOfTreeTargetsExistenceToFilterTargets].
static const bool kOnlyUseTipOfTreeTargetsExistenceToFilterTargets = false;
Comment on lines +46 to +47
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than exposing the default and maybe hiding places that do not pass this flag (error prone); make people refer to config.onlyUseTipOfTreeTargetsExistenceToFilterTargets.


/// When present on a pull request, instructs Cocoon to submit it
/// automatically as soon as all the required checks pass.
///
Expand Down Expand Up @@ -514,9 +523,19 @@ final class DynamicConfig {

final ContentAwareHashingJson contentAwareHashing;

/// Whether to _only_ use the existence of a target at tip-of-tree to filter
/// out targets on other branches.
///
/// See [CiYaml.onlyUseTipOfTreeTargetsExistenceToFilterTargets].
@JsonKey(
defaultValue: Config.kOnlyUseTipOfTreeTargetsExistenceToFilterTargets,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

default this to true as that's the current/safe option. The flag should get flipped in another PR and then eventually cleaned up in the future.

)
final bool onlyUseTipOfTreeTargetsExistenceToFilterTargets;
Comment on lines 525 to +533
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I can see us having some other flags for ci_yaml; can we create a CiYamlFlags like ContentAwareHashingJson instead of expanding this?


DynamicConfig({
required this.backfillerCommitLimit,
required this.contentAwareHashing,
required this.onlyUseTipOfTreeTargetsExistenceToFilterTargets,
});

/// Connect the generated [_$DynamicConfigFromJson] function to the `fromJson`
Expand Down
20 changes: 12 additions & 8 deletions app_dart/lib/src/service/config.g.dart

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

7 changes: 6 additions & 1 deletion app_dart/lib/src/service/scheduler/ci_yaml_fetcher.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import '../firestore.dart';
interface class CiYamlFetcher {
/// Creates a [CiYamlFetcher] from the provided configuration.
CiYamlFetcher({
required Config config,
required CacheService cache,
required FirestoreService firestore,
HttpClientProvider httpClientProvider = Providers.freshHttpClient,
Expand All @@ -30,13 +31,15 @@ interface class CiYamlFetcher {
delayFactor: Duration(seconds: 2),
maxAttempts: 4,
),
}) : _cache = cache,
}) : _config = config,
_cache = cache,
_cacheTtl = cacheTtl,
_subcacheName = subcacheName,
_retryOptions = retryOptions,
_httpClientProvider = httpClientProvider,
_firestore = firestore;

final Config _config;
final CacheService _cache;
final String _subcacheName;
final Duration _cacheTtl;
Expand Down Expand Up @@ -127,6 +130,8 @@ interface class CiYamlFetcher {
branch: commit.branch,
totConfig: totCiYaml,
validate: validate,
onlyUseTipOfTreeTargetsExistenceToFilterTargets:
_config.flags.onlyUseTipOfTreeTargetsExistenceToFilterTargets,
);
}

Expand Down
24 changes: 24 additions & 0 deletions app_dart/test/service/scheduler/ci_yaml_fetcher_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import 'package:path/path.dart' as p;
import 'package:retry/retry.dart';
import 'package:test/test.dart';

import '../../src/fake_config.dart';
import '../../src/service/fake_firestore_service.dart';
import '../../src/utilities/entity_generators.dart';

Expand All @@ -34,6 +35,7 @@ void main() {
late CacheService cache;
late MockClient httpClient;
late FakeFirestoreService firestore;
late FakeConfig config;

late CiYamlFetcher ciYamlFetcher;

Expand All @@ -46,6 +48,7 @@ void main() {
firestore = FakeFirestoreService();

ciYamlFetcher = CiYamlFetcher(
config: config = FakeConfig(),
cache: cache,
httpClientProvider: () => httpClient,
retryOptions: const RetryOptions(maxAttempts: 1),
Expand Down Expand Up @@ -77,6 +80,27 @@ void main() {
);
}

// TODO(matanlurey): Remove after https://github.com/flutter/flutter/issues/169625.
test('passes flag onlyUseTipOfTreeTargetsExistenceToFilterTargets', () async {
config.dynamicConfig = DynamicConfig.fromJson({
'onlyUseTipOfTreeTargetsExistenceToFilterTargets': false,
});
mockFillFirestore(slug: Config.packagesSlug, branch: 'main');
httpClient = MockClient((request) async {
return http.Response(singleCiYaml, HttpStatus.ok);
});

final ciYaml = await ciYamlFetcher.getCiYamlByCommit(
CommitRef(slug: Config.packagesSlug, sha: currentSha, branch: 'main'),
);
expect(
ciYaml
.configs[CiType.any]
?.onlyUseTipOfTreeTargetsExistenceToFilterTargets,
isFalse,
);
});

test('fetches the root .ci.yaml for a repository (GitHub)', () async {
httpClient = MockClient((request) async {
if (request.url.host != 'raw.githubusercontent.com') {
Expand Down
6 changes: 5 additions & 1 deletion app_dart/tool/local_server.dart
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ Future<void> main() async {
/// Github checks api service used to provide luci test execution status on the Github UI.
final githubChecksService = GithubChecksService(config);

final ciYamlFetcher = CiYamlFetcher(cache: cache, firestore: firestore);
final ciYamlFetcher = CiYamlFetcher(
config: config,
cache: cache,
firestore: firestore,
);

/// Cocoon scheduler service to manage validating commits in presubmit and postsubmit.
final scheduler = Scheduler(
Expand Down