-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retrieve changes by branch #7090
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Add the possibility to retrieve the last changes for each branch. Only retrieve the changes that have an associated buildset.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I see. Probably it does't make sense to always fetch them. But can it be at least added when |
||
change['changeid'] | ||
) | ||
else: | ||
sskey = ('sourcestamps', str(change['sourcestampid'])) | ||
change['sourcestamp'] = yield self.master.data.get(sskey) | ||
|
@@ -104,8 +107,14 @@ def get(self, resultSpec, kwargs): | |
changes = [] | ||
else: | ||
if resultSpec is not None: | ||
resultSpec.fieldMapping = self.fieldMapping | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be missing something, but why can't we use fieldMapping for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
changes = yield self.master.db.changes.getChanges(resultSpec=resultSpec) | ||
branch = resultSpec.popStringFilter('branch') | ||
if branch is not None: | ||
changes = yield self.master.db.changes.getChangesForBranch( | ||
branch, resultSpec.order, resultSpec.limit | ||
) | ||
else: | ||
resultSpec.fieldMapping = self.fieldMapping | ||
changes = yield self.master.db.changes.getChanges(resultSpec=resultSpec) | ||
results = [] | ||
for ch in changes: | ||
results.append((yield self._fixChange(ch, is_graphql='graphql' in kwargs))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?