Retrieve changes by branch #7090

vladbogo · 2023-09-06T14:10:44Z

Add the possibility to retrieve the last changes for each branch. Only retrieve the changes that have an associated buildset.

Contributor Checklist:

I have updated the unit tests
I have created a file in the newsfragments directory (and read the README.txt in that directory)
I have updated the appropriate documentation

vladbogo · 2023-09-06T14:21:27Z

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:

what tests are expected for such a change? For the DB part, should I just create some changes with buildsets and retrieve those?

p12tic · 2023-09-07T18:26:16Z

master/buildbot/db/changes.py

+            rp.close()
+            return list(changeids)
+
+        d = self.db.pool.do(thd)


Use yield here and @defer.inlineCallbacks on getChangesForBranch.

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?

p12tic · 2023-09-07T18:26:34Z

master/buildbot/db/changes.py

+            rp = conn.execute(q)
+            changeids = [self._getDataFromRow(row) for row in rp]
+            rp.close()
+            return list(changeids)


I think changeids is already a list.

p12tic · 2023-09-07T18:27:47Z

master/buildbot/data/changes.py

@@ -104,8 +105,12 @@ def get(self, resultSpec, kwargs):
                changes = []
        else:
            if resultSpec is not None:
-                resultSpec.fieldMapping = self.fieldMapping


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.

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.

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?

p12tic · 2023-09-07T18:28:32Z

master/buildbot/data/changes.py

@@ -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'])


Just out of interest, are you using the graphql API?

no, not using graphql

p12tic · 2023-09-07T18:29:50Z

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.

p12tic · 2023-09-24T20:11:34Z

master/buildbot/data/changes.py

@@ -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(


I think this will be slow when retrieving many changes. Let's skip it for now, especially if you aren't using graphql.

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.

p12tic · 2023-09-24T20:13:20Z

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

vladbogo · 2023-09-25T13:33:08Z

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 branch is specified which I agree is not the cleanest way. I am not sure if there is a better/easier way to achieve this. Do you think a different endpoint would be more suitable?

vladbogo · 2023-09-25T13:38:26Z

master/buildbot/data/buildsets.py

-        d = self.master.db.buildsets.getBuildsets(
-            complete=complete, resultSpec=resultSpec)
+
+        branch = resultSpec.popStringFilter('branch')


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?

vladbogo · 2023-11-16T11:19:05Z

@p12tic any thoughts on the above comments?

p12tic · 2023-12-05T02:43:06Z

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..

vladbogo · 2023-12-05T16:14:52Z

I see. Please let me know if I can contribute in any way

vladbogo · 2024-04-18T08:11:10Z

@p12tic are there any updates on this? Is there anything I can help with?

vladbogo force-pushed the filter_changes_by_branch branch from ebd6da6 to 08f5e9d Compare September 6, 2023 14:44

p12tic reviewed Sep 7, 2023

View reviewed changes

Retrieve changes by branch

106b38e

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 September 18, 2023 11:29

p12tic reviewed Sep 24, 2023

View reviewed changes

vladbogo commented Sep 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve changes by branch #7090

Retrieve changes by branch #7090

vladbogo commented Sep 6, 2023 •

edited

Loading

vladbogo commented Sep 6, 2023

p12tic Sep 7, 2023

vladbogo Sep 18, 2023

p12tic Sep 7, 2023

vladbogo Sep 18, 2023

p12tic Sep 7, 2023

vladbogo Sep 18, 2023

vladbogo Sep 25, 2023

p12tic Sep 7, 2023

vladbogo Sep 18, 2023

p12tic commented Sep 7, 2023

p12tic Sep 24, 2023

vladbogo Sep 25, 2023

p12tic commented Sep 24, 2023

vladbogo commented Sep 25, 2023 •

edited

Loading

vladbogo Sep 25, 2023

vladbogo commented Nov 16, 2023

p12tic commented Dec 5, 2023

vladbogo commented Dec 5, 2023

vladbogo commented Apr 18, 2024 •

edited

Loading

Retrieve changes by branch #7090

Are you sure you want to change the base?

Retrieve changes by branch #7090

Conversation

vladbogo commented Sep 6, 2023 • edited Loading

Contributor Checklist:

vladbogo commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p12tic commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p12tic commented Sep 24, 2023

vladbogo commented Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

vladbogo commented Nov 16, 2023

p12tic commented Dec 5, 2023

vladbogo commented Dec 5, 2023

vladbogo commented Apr 18, 2024 • edited Loading

vladbogo commented Sep 6, 2023 •

edited

Loading

vladbogo commented Sep 25, 2023 •

edited

Loading

vladbogo commented Apr 18, 2024 •

edited

Loading