Create /api/get-branches to support frontend listing branches by keyonghan · Pull Request #682 · flutter/cocoon

keyonghan · 2020-03-10T23:51:22Z

This is to support dashboard to list branches by returning the available matched ones.

CaseyHillers

Thanks for working on this Keyong!

CaseyHillers · 2020-03-11T00:43:57Z

app_dart/bin/server.dart

        delegate: GetStatus(config),
      ),
      '/api/public/get-timeseries-history': GetTimeSeriesHistory(config),
+      '/api/public/get-branches': GetBranches(config),


We should consider caching this endpoint.

Yeah, good point. BTW: the dashboard sometimes shows red when all tasks seem to be successful. Is it due to the caching delay? If so, maybe good to consider reducing the refreshing period.

CaseyHillers · 2020-03-11T00:44:14Z

app_dart/lib/src/request_handlers/get_branches.dart

@@ -0,0 +1,65 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.


CaseyHillers · 2020-03-11T00:46:47Z

app_dart/lib/src/request_handlers/get_branches.dart

+/// Signature for a function that calculates the backoff duration to wait in
+/// between requests when GitHub responds with an error.
+///
+/// The `attempt` argument is zero-based, so if the first attempt to request


nit: in dartdocs I believe it would be formatted as $attempt since it's a variable

Did a quick search, seems we have been using [] instead of $. Changed.

CaseyHillers · 2020-03-11T00:48:37Z

app_dart/lib/src/request_handlers/get_branches.dart

+  @override
+  Future<Body> get() async {
+    final GitHub github = await config.createGitHubClient();
+    const RepositorySlug slug = RepositorySlug('flutter', 'flutter');


We should consider storing the flutter/flutter slug in the config so the specifics aren't littered throughout the code.

CaseyHillers · 2020-03-11T00:49:56Z

app_dart/lib/src/request_handlers/get_branches.dart

+    final GitHub github = await config.createGitHubClient();
+    const RepositorySlug slug = RepositorySlug('flutter', 'flutter');
+    final Stream<Branch> branchList = github.repositories.listBranches(slug);
+    final List<String> regExps = await loadBranchRegExps(


Where is loadBranchRegExps defined?

It was originally defined in refresh_github_commits.dart as a private function. To share/reduce the codes, make it public so the API here can call that function as well.

Moved to utils.dart.

CaseyHillers · 2020-03-11T00:54:15Z

app_dart/lib/src/request_handlers/get_branches.dart

+  return const Duration(seconds: 2) * (attempt + 1);
+}
+
+/// Queries GitHub for the list of all available branches, and returns those


nit: "list of all available branches on the flutter/flutter repo"
nit: "and returns List of branches"

CaseyHillers · 2020-03-11T00:57:50Z

app_dart/lib/src/request_handlers/refresh_github_commits.dart

-    const String path =
-        '/flutter/cocoon/master/app_dart/dev/branch_regexps.txt';
-    final Uri url = Uri.https('raw.githubusercontent.com', path);
+Future<List<String>> loadBranchRegExps(


Seems this is used between this file and get-branches handler. We should consider moving it to something similar to github/utils.dart to reuse the logic between the two files and test it once.

CaseyHillers · 2020-03-11T01:00:53Z

app_dart/lib/src/request_handlers/refresh_github_commits.dart

+    HttpClientProvider branchHttpClientProvider,
+    Logging log,
+    GitHubBackoffCalculator gitHubBackoffCalculator) async {
+  const String path = '/flutter/cocoon/master/app_dart/dev/branch_regexps.txt';


Side note, doesn't have to be addressed this PR: Was there a reason the branch regexps weren't just stored in the cocoon config? Seems like it would save us this extra HTTP query.

HTTP request is intentional: this enables instant access without app re-deployment when some changes are made.

CaseyHillers · 2020-03-11T01:04:16Z

app_dart/test/request_handlers/get_branches_test.dart

@@ -0,0 +1,79 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.


CaseyHillers · 2020-03-11T01:10:11Z

app_dart/test/request_handlers/get_branches_test.dart

+      });
+    });
+
+    test('returns branches matching regExps', () async {


There's a lot of different try/catch if/else logic blocks that I believe should be tested here. I think moving the loadRegExp to a separate util file would make it easier to focus on this logic for a test.

moved to utils.dart and added corresponding tests.

keyonghan · 2020-03-11T17:47:02Z

Thanks for working on this Keyong!

@CaseyHillers Thank you for the detailed comments!

CaseyHillers

LGTM with nits

CaseyHillers · 2020-03-11T17:49:51Z

app_dart/lib/src/request_handlers/get_branches.dart

-
-/// Queries GitHub for the list of all available branches, and returns those
+/// Queries GitHub for the list of all available branches on
+/// [flutter/flutter] repo, and returns list of branches


nit: flutter/flutter isn't a variable. Should it be [config.flutterSlug]?

CaseyHillers · 2020-03-11T17:50:30Z

app_dart/lib/src/request_handlers/utils.dart

+/// Signature for a function that calculates the backoff duration to wait in
+/// between requests when GitHub responds with an error.
+///
+/// The `attempt` argument is zero-based, so if the first attempt to request


nit: dartdocs [attempt]

CaseyHillers · 2020-03-11T17:50:36Z

app_dart/lib/src/request_handlers/utils.dart

+///
+/// The `attempt` argument is zero-based, so if the first attempt to request
+/// from GitHub fails, and we're backing off before making the second attempt,
+/// the `attempt` argument will be zero.


CaseyHillers · 2020-03-11T17:52:36Z

app_dart/lib/src/request_handlers/utils.dart

+        log.error(
+            'Attempt to download branch_regexps.txt failed:\n$error\n$stackTrace');
+      }
+      await Future<void>.delayed(gitHubBackoffCalculator(attempt));


We should wait for #683 to land and take advantage of that logic instead of reimplementing it here.

Quite some other existing logics exist in cocoon can be applied with #683

It would be good to have an issue to track: flutter/flutter#52427. So merge here and make the bunch updates later.

create get-branches API

fc6a23a

keyonghan requested review from CaseyHillers, godofredoc and tvolkert March 10, 2020 23:52

CaseyHillers reviewed Mar 11, 2020

View reviewed changes

put common scripts to utils

8382072

CaseyHillers approved these changes Mar 11, 2020

View reviewed changes

comments udpate

c6e4522

keyonghan merged commit 0bfe29a into flutter:master Mar 11, 2020

keyonghan mentioned this pull request Mar 11, 2020

Support branching for flutter repo flutter/flutter#51807

Closed

19 tasks

keyonghan deleted the get_branch_list branch March 13, 2024 20:16

		@@ -0,0 +1,65 @@
		// Copyright 2019 The Chromium Authors. All rights reserved.

		@@ -0,0 +1,79 @@
		// Copyright 2019 The Chromium Authors. All rights reserved.

Conversation

keyonghan commented Mar 10, 2020

Uh oh!

CaseyHillers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonghan commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CaseyHillers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

keyonghan commented Mar 11, 2020 •

edited

Loading