Support branch parameter for API get-status #668
Conversation
CaseyHillers
left a comment
There was a problem hiding this comment.
Should we add support for the tree status to be based on branch too?
There was a problem hiding this comment.
Please refer to the last test in get_status_test.dart
There was a problem hiding this comment.
Doesn't have to be this PR due to the priority, but we should start moving some of the datastore logic we do to be tested in datastore_test.dart. datastore_test.dart doesn't even exist :)
There was a problem hiding this comment.
What about a test for when branch isn't given?
There was a problem hiding this comment.
Updated the commit model to have branch with default value `master.
There was a problem hiding this comment.
Does the datastore data have all tasks currently set to master if they're null? Otherwise wouldn't the model get overwritten from that null in datastore.
There was a problem hiding this comment.
I guess you meant commit, rather than task. Yes, if null, master is the default value now. (just pushed the changes)
There was a problem hiding this comment.
Was there an update to the http library?
|
Rebase with master, #670 should have fixed the issues LUCI is showing. |
app_dart/index.yaml
Outdated
There was a problem hiding this comment.
This will not create indexes for previous data. Is there a plan to create these indexes?
Explanation: https://stackoverflow.com/questions/35742786/how-to-add-a-composite-index-in-google-datastore
There was a problem hiding this comment.
In the past we've just had the person who updated the yaml file run the gcloud command to create the index.
There was a problem hiding this comment.
This was follow up from an offline discussion Keyong and I had. This is in reference to that the composite index will only be created on task entities that are saved once this index is pushed. Older task entities would no longer be served unless there was a script that went ahead and resaved each one to have this index created for them.
There was a problem hiding this comment.
As talked about updating old data with 'master' below with Todd, this is of lower priority as long as those data is old enough, which actually is. I am gonna to merge this PR for now.
Created flutter/flutter#52066 to track.
Thanks for the fix! |
There was a problem hiding this comment.
Did we back-fill existing data to add 'master' as the branch?
There was a problem hiding this comment.
Not yet. Frontend is using latest 30 commits for status calculation. Unless people keep scrolling down the dashboard, we have enough data for status query at this moment (now we have 45 commits with master as the branch in datastore).
Do you have any suggestion to update old data efficiently? Didn't find any bunch update options. I am thinking about to write some scripts to walk through all records and update them sequentially.
There was a problem hiding this comment.
I had written a debug handler for updating data at one point for this purpose, but never checked it in for security reasons...
As you say, it's lower priority if the affected rows are old enough to not show on the dashboard by default.
Yes, we should. I think we also need to update the cirrus/luci/devicelab test status logic to be based on branch as well. I updated flutter/flutter#51807 to track This should be fixing flutter/flutter#51884 |
a039365 to
4394a48
Compare
This PR prepares for frontend support to list commits w.r.t. different branches. Default lists are for
master, otherwise it shows corresponding lists whenbranchparameter is specified.Related issue: flutter/flutter#51807