[dashboard] Support multiple repos#1514
Conversation
dashboard/lib/state/build.dart
Outdated
There was a problem hiding this comment.
This doesn't need to be run every 30 seconds as this is almost never changes (order of once a month)
|
@goderbauer can you take a look on the Navigator side of this PR, and let me know if there's a better way of passing around query parameters? |
There was a problem hiding this comment.
This looks like a strange side effect from build.
There was a problem hiding this comment.
So, the branch and repository information get passed twice? Once in the name of the route and once as arguments object?
There was a problem hiding this comment.
It looks like "onGenerateRoute" just ignores the arguments, though?
There was a problem hiding this comment.
Is there a better way of passing the args from onGenerateRoute? I'm doing
repo: uriData.queryParameters['repo'],
branch: uriData.queryParameters['branch'],
to initialize the fields based on what was passed from RouteSettings
There was a problem hiding this comment.
I removed the settings, but I still needed to pass RouteSettings.name in onGenerateRoute so the URL would be updated
CaseyHillers
force-pushed
the
dashboard-repo
branch
from
0a21dd1 to
ac9d035
Compare
|
Friendly ping now that the code freeze is over
|
| if (branch == 'master' || branch == 'main') { | ||
| branch = defaultBranches[repo]; | ||
| } |
There was a problem hiding this comment.
what is the use case of this?
There was a problem hiding this comment.
By default, you land on flutter/master. If you were to swap the repo from flutter->engine, you would also need to change the branch from master->main.
| if (valueMap != null) { | ||
| for (final MapEntry<String, String> mapEntry in valueMap.entries) { | ||
| _allProperties[mapEntry.key].stringValue = mapEntry.value; | ||
| if (_allProperties.containsKey(mapEntry.key)) { |
There was a problem hiding this comment.
is this condition needed, what are cases where unexpected keys exist?
There was a problem hiding this comment.
This is searching all the query parameters. Since this PR adds repo and branch query params, this needed to be updated to no-op on unknown keys.
| 'Key': task.key.child.name, | ||
| // This prepares this api to support different repos. | ||
| 'Owner': 'flutter', | ||
| 'Repo': 'flutter', | ||
| })); |
There was a problem hiding this comment.
Is this intentional? Don't we want to support rerun on other repos?
There was a problem hiding this comment.
Passing the task key is sufficient since now every repo's builds are stored in Cocoon's datastore
keyonghan
left a comment
keyonghan
left a comment
There was a problem hiding this comment.
LGTM from infra side.
Leave the approval to @goderbauer re Navigator side codes.
| repo = widget.queryParameters['repo'] ?? 'flutter'; | ||
| branch = widget.queryParameters['branch'] ?? 'master'; |
There was a problem hiding this comment.
nit: you could remove the "?? 'flutter'" and "?? 'master'" here since that's duplicated in line 56/57 below.
CaseyHillers
added
the
waiting for tree to go green
This reverts commit 9e72e31.
4 participants
Fixes flutter/flutter#53714
Fixes flutter/flutter#94522
Changes
updateCurrentBranchto beupdateCurrentRepoBranchas it always requires a full flush of the dataTest Env
https://testchillers-dot-flutter-dashboard.appspot.com/#/build?repo=engine&branch=main
Go to settings, then change the repository.