[cloud functions] Copy approvals from a Gerrit review when it lands
Change-Id: I41d88a2cc8d877f70d499561c29c511280f59f87
Bug: https://github.com/dart-lang/dart_ci/issues/48
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/126880
Reviewed-by: Alexander Thomas <athom@google.com>
Reviewed-by: Jonas Termansen <sortie@google.com>
diff --git a/functions/node/builder.dart b/functions/node/builder.dart
index 75e095c..ccb97be 100644
--- a/functions/node/builder.dart
+++ b/functions/node/builder.dart
@@ -57,6 +57,7 @@
int buildNumber;
int startIndex;
int endIndex;
+ Set<String> tryApprovals;
Statistics stats = Statistics();
@@ -93,6 +94,12 @@
}
}
endIndex = commit['index'];
+ if (commit.containsKey('review')) {
+ tryApprovals = {
+ for (final result in await firestore.tryApprovals(commit['review']))
+ testResult(result)
+ };
+ }
// If this is a new builder, use the current commit as a trivial blamelist.
if (firstResult['previous_commit_hash'] == null) {
startIndex = endIndex;
@@ -102,8 +109,11 @@
}
}
+ /// This function is idempotent, so every call of it should write the
+ /// same info to new Firestore documents. It is save to call multiple
+ /// times simultaneously.
Future<void> getMissingCommits() async {
- final lastCommit = await firestore.getLastCommit(commitHash);
+ final lastCommit = await firestore.getLastCommit();
final lastHash = lastCommit['id'];
final lastIndex = lastCommit['index'];
@@ -137,16 +147,32 @@
}
var index = lastIndex + 1;
for (Map<String, dynamic> commit in commits.reversed) {
+ final review = _review(commit);
await firestore.addCommit(commit['commit'], {
'author': commit['author']['email'],
'created': parseGitilesDateTime(commit['committer']['time']),
'index': index,
- 'title': commit['message'].split('\n').first
+ 'title': commit['message'].split('\n').first,
+ if (review != null) 'review': review
});
+ if (review != null) {
+ await landReview(commit, index);
+ }
++index;
}
}
+ /// This function is idempotent and may be called multiple times
+ /// concurrently.
+ Future<void> landReview(Map<String, dynamic> commit, int index) async {
+ int review = _review(commit);
+ // Optimization to avoid duplicate work: if another instance has linked
+ // the review to its landed commit, do nothing.
+ if (await firestore.reviewIsLanded(review)) return;
+ await firestore.linkReviewToCommit(review, index);
+ await firestore.linkCommentsToCommit(review, index);
+ }
+
Future<void> storeConfigurationsInfo(Iterable<String> configurations) async {
for (final configuration in configurations) {
await firestore.updateConfiguration(configuration, builderName);
@@ -154,11 +180,29 @@
}
Future<void> storeChange(Map<String, dynamic> change) async {
- firestore.storeChange(change, startIndex, endIndex);
+ firestore.storeChange(change, startIndex, endIndex,
+ approved: tryApprovals?.contains(testResult(change)) ?? false);
stats.changes++;
}
}
+final reviewRegExp = RegExp(
+ '^Reviewed-on: https://dart-review.googlesource.com/c/sdk/\\+/(\\d+)\$',
+ multiLine: true);
+
+int _review(Map<String, dynamic> commit) {
+ final match = reviewRegExp.firstMatch(commit['message']);
+ if (match != null) return int.parse(match.group(1));
+ return null;
+}
+
+String testResult(Map<String, dynamic> change) => [
+ change['name'],
+ change['result'],
+ change['previous_result'],
+ change['expected']
+ ].join(' ');
+
class Statistics {
int results = 0;
int changes = 0;
diff --git a/functions/node/firestore.dart b/functions/node/firestore.dart
index 36c99fd..8bea531 100644
--- a/functions/node/firestore.dart
+++ b/functions/node/firestore.dart
@@ -11,7 +11,7 @@
Future<Map<String, dynamic>> getCommit(String hash);
- Future<Map<String, dynamic>> getLastCommit(String hash);
+ Future<Map<String, dynamic>> getLastCommit();
Future<void> addCommit(String id, Map<String, dynamic> data);
@@ -20,7 +20,8 @@
Future<void> updateBuildInfo(String builder, int buildNumber, int index);
Future<void> storeChange(
- Map<String, dynamic> change, int startIndex, int endIndex);
+ Map<String, dynamic> change, int startIndex, int endIndex,
+ {bool approved});
Future<void> storeTryChange(
Map<String, dynamic> change, int review, int patchset);
@@ -28,4 +29,12 @@
Future<void> storeReview(String review, Map<String, dynamic> data);
Future<void> storePatchset(
String review, String patchset, Map<String, dynamic> data);
+
+ Future<bool> reviewIsLanded(int review);
+
+ Future<void> linkReviewToCommit(int review, int index);
+
+ Future<void> linkCommentsToCommit(int review, int index);
+
+ Future<List<Map<String, dynamic>>> tryApprovals(int review);
}
diff --git a/functions/node/firestore_impl.dart b/functions/node/firestore_impl.dart
index 53806e5..4d1ae51 100644
--- a/functions/node/firestore_impl.dart
+++ b/functions/node/firestore_impl.dart
@@ -36,7 +36,7 @@
.get()
.then((s) => s.exists ? s.data.toMap() : null);
- Future<Map<String, dynamic>> getLastCommit(String hash) async {
+ Future<Map<String, dynamic>> getLastCommit() async {
QuerySnapshot lastCommit = await firestore
.collection('commits')
.orderBy('index', descending: true)
@@ -86,7 +86,8 @@
}
Future<void> storeChange(
- Map<String, dynamic> change, int startIndex, int endIndex) async {
+ Map<String, dynamic> change, int startIndex, int endIndex,
+ {bool approved = false}) async {
String name = change['name'];
String result = change['result'];
String previousResult = change['previous_result'] ?? 'new test';
@@ -120,7 +121,8 @@
'expected': change['expected'],
'blamelist_start_index': startIndex,
'blamelist_end_index': endIndex,
- 'configurations': <String>[change['configuration']]
+ 'configurations': <String>[change['configuration']],
+ if (approved) 'approved': true
}));
}
@@ -188,4 +190,40 @@
firestore
.document('reviews/$review/patchsets/$patchset')
.setData(DocumentData.fromMap(data));
+
+ /// Returns true if a review record in the database has a landed_index field,
+ /// or if there is no record for the review in the database. Reviews with no
+ /// test failures have no record, and don't need to be linked when landing.
+ Future<bool> reviewIsLanded(int review) =>
+ firestore.document('reviews/$review').get().then((document) =>
+ !document.exists || document.data.getInt('landed_index') != null);
+
+ Future<void> linkReviewToCommit(int review, int index) => firestore
+ .document('reviews/$review')
+ .updateData(UpdateData.fromMap({'landed_index': index}));
+
+ Future<void> linkCommentsToCommit(int review, int index) async {
+ QuerySnapshot comments = await firestore
+ .collection('comments')
+ .where('review', isEqualTo: review)
+ .get();
+ if (comments.isEmpty) return;
+ final batch = firestore.batch();
+ for (final comment in comments.documents) {
+ batch.updateData(
+ comment.reference,
+ UpdateData.fromMap(
+ {'blamelist_start_index': index, 'blamelist_end_index': index}));
+ }
+ await batch.commit();
+ }
+
+ Future<List<Map<String, dynamic>>> tryApprovals(int review) async {
+ QuerySnapshot approvals = await firestore
+ .collection('try_results')
+ .where('approved', isEqualTo: true)
+ .where('review', isEqualTo: review)
+ .get();
+ return [for (final document in approvals.documents) document.data.toMap()];
+ }
}
diff --git a/functions/node/test/test.dart b/functions/node/test/test.dart
index 85a83fe..dcbe90a 100644
--- a/functions/node/test/test.dart
+++ b/functions/node/test/test.dart
@@ -2,8 +2,6 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
-import 'dart:convert';
-
import 'package:mockito/mockito.dart';
import 'package:test/test.dart';
@@ -15,7 +13,6 @@
test("Store already existing commit", () async {
final firestore = FirestoreServiceMock();
final change = alreadyExistingCommitChange;
- final hash = change['commit_hash'];
final builder = Build(existingCommitHash, change, firestore);
await builder.storeBuildCommitsInfo();
diff --git a/results_feed/doc/results_feed.proto b/results_feed/doc/results_feed.proto
index fd30aac..4579018 100644
--- a/results_feed/doc/results_feed.proto
+++ b/results_feed/doc/results_feed.proto
@@ -102,6 +102,7 @@
string author = 3; // The author of the commit, as an email address.
google.protobuf.Timestamp created = 4;
string title = 5; // The first line of the commit message.
+ int32 review = 6; // If the commit is a landed Gerrit review, its number.
}
// A map from configuration names to builder names.
@@ -130,7 +131,8 @@
int32 patchset_group = 5;
}
- repeated Patchset patchsets
+ repeated Patchset patchsets = 3;
+ int32 landed_index = 4; // When a review lands, this is the index of the landed commit.
}
message Comment {
