Retrieve changes by branch #7090
Conversation
|
Hi. I have created this draft PR in order to add the functionality for being able to retrieve the latest changes for a particular branch. It only retrieves changes that have a buildset associated since if I am not mistaken there might be changes that do not have one. It would be great to get an initial feedback and I also have a question, mainly related to tests:
|
vladbogo
force-pushed
the
filter_changes_by_branch
branch
from
ebd6da6
to
08f5e9d
Compare
master/buildbot/db/changes.py
Outdated
| rp.close() | ||
| return list(changeids) | ||
|
|
||
| d = self.db.pool.do(thd) |
There was a problem hiding this comment.
Use yield here and @defer.inlineCallbacks on getChangesForBranch.
There was a problem hiding this comment.
not sure if it's correct in the new form since I think it will only return the changeids. Or am I wrong? Any suggestions?
master/buildbot/db/changes.py
Outdated
| rp = conn.execute(q) | ||
| changeids = [self._getDataFromRow(row) for row in rp] | ||
| rp.close() | ||
| return list(changeids) |
There was a problem hiding this comment.
I think changeids is already a list.
| @@ -104,8 +105,12 @@ def get(self, resultSpec, kwargs): | |||
| changes = [] | |||
| else: | |||
| if resultSpec is not None: | |||
| resultSpec.fieldMapping = self.fieldMapping | |||
There was a problem hiding this comment.
I may be missing something, but why can't we use fieldMapping for branch is not None case? Other potential filters may still be relevant.
There was a problem hiding this comment.
Yes, I think other fields may be relevant. However, based on the table joins, some of them might have a different result which might be confusing since it will filter all the entries for a particular branch. Also, some of them might be trickier to integrate. I'll look into this in more detail.
There was a problem hiding this comment.
Based on further looking, I think it should be possible to use the resultSpec, but that would be on top of the join between the changes and buildset tables, so some filters might have a different meaning between the case where 'branch' is specified or not. Is this ok?
master/buildbot/data/changes.py
Outdated
| @@ -42,6 +42,7 @@ def _fixChange(self, change, is_graphql): | |||
| {'name': k, 'source': v[1], 'value': json.dumps(v[0])} | |||
| for k, v in props.items() | |||
| ] | |||
| change['builds'] = yield self.master.db.builds.getBuildsForChange(change['changeid']) | |||
There was a problem hiding this comment.
Just out of interest, are you using the graphql API?
There was a problem hiding this comment.
no, not using graphql
|
For now I've commented on some trivial things that I noticed. For your questions I will come back a bit later. |
Add the possibility to retrieve the last changes for each branch. Only retrieve the changes that have an associated buildset.
vladbogo
force-pushed
the
filter_changes_by_branch
branch
from
08f5e9d
to
106b38e
Compare
| @@ -42,6 +42,9 @@ def _fixChange(self, change, is_graphql): | |||
| {'name': k, 'source': v[1], 'value': json.dumps(v[0])} | |||
| for k, v in props.items() | |||
| ] | |||
| change['builds'] = yield self.master.db.builds.getBuildsForChange( | |||
There was a problem hiding this comment.
I think this will be slow when retrieving many changes. Let's skip it for now, especially if you aren't using graphql.
There was a problem hiding this comment.
Ok, I see. Probably it does't make sense to always fetch them. But can it be at least added when branch is specified? The reason the builds are needed is to directly show them in the grid view without any additional calls. Maybe they can be merged in a single query. What do you think would be the best approach? Also, if there are two many changes, we can leave it out for now and I'll work on a new PR to add this.
|
I looked more into the PR and now I wonder whether existing code would support the use case for retrieving changes for branch. When checking the buildbot API it already supports retrieving changes by branch. For example: https://buildbot.buildbot.net/api/v2/changes?limit=50&branch=refs/pull/XYZ/merge&order=when_timestamp. I see that when_timestamp is incorrect when it's filled by data from Github, so this needs additional investigation. But basic functionality is already there, don't you think? For retrieving buildsets for branch, there's no such functionality, so that part could go in. For tests, what you wrote makes sense. For an example, you can check tests in master/buildbot/test/unit/data/test_buildsets.py |
|
Yes, you are right. However, if I am not mistaken, this will include changes that don't have a build associated. I think that changes that do not have actual buildbot runs can be added only when a push contains multiple commits. So, in this case, you would get the latest X changes for a branch, but from a grid view perspective, you won't be able to get the latest X changes that actually have a buildbot run. Basically what the PR adds, is the join with the buildset table, that ensure that each returned change, has a build associated. And it only does this if the |
| d = self.master.db.buildsets.getBuildsets( | ||
| complete=complete, resultSpec=resultSpec) | ||
|
|
||
| branch = resultSpec.popStringFilter('branch') |
There was a problem hiding this comment.
Here I am not sure if branch can be a specified field in the resultSpec. Since Buildset doesn't have a branch field, I think this might be an issue. In my custom run, I have also added a new entry in the fieldMapping but that translates into failing tests. So, can you please confirm if a resultSpec is limited to having exactly the same fields as the Buildset or not?
|
@p12tic any thoughts on the above comments? |
@vladbogo Sorry for forgetting about this PR. I thought it makes sense for me to look into adding this feature into the existing API, but didn't have time for that yet.. |
|
I see. Please let me know if I can contribute in any way |
|
@p12tic are there any updates on this? Is there anything I can help with? |
Add the possibility to retrieve the last changes for each branch. Only retrieve the changes that have an associated buildset.
Contributor Checklist:
newsfragmentsdirectory (and read theREADME.txtin that directory)