Support `merged` pull requests for `github` and `gitlab` #311

cardil · 2023-07-26T15:54:29Z

Changes

Listing merged pull requests on Github
Listing merged merge requests on Gitlab

psss · 2023-07-27T10:00:19Z

Thanks for the pull request. Actually, the PullRequestsClosed stats are intended for listing merged pull requests:

* Pull requests closed on github: 9
    * teemtee/tmt#2221 - Fix test checking custom destination for libraries
    * teemtee/tmt#2216 - Create plans to cover individual step features
    * teemtee/tmt#2206 - Group discover/fmf options, improve wording a bit
    * teemtee/tmt#2203 - Add template option to polarion report
    * teemtee/tmt#2178 - Add cache_property for things that are generated...
    * teemtee/tmt#2173 - Simplify public git conversion with a declarative list
    * teemtee/tmt#2172 - Clean up logging in `tmt.utils.create_directory()`
    * teemtee/tmt#2171 - Spec-based container becomes generic over input...
    * teemtee/tmt#2160 - Move test framework code into distinct framework...

* Pull requests reviewed on github: 6
    * teemtee/tmt#2203 - Add template option to polarion report
    * teemtee/tmt#2178 - Add cache_property for things that are generated...
    * teemtee/tmt#2173 - Simplify public git conversion with a declarative list
    * teemtee/tmt#2172 - Clean up logging in `tmt.utils.create_directory()`
    * teemtee/tmt#2171 - Spec-based container becomes generic over input...
    * teemtee/tmt#2160 - Move test framework code into distinct framework...

* Merged pull requests on github: 3
    * teemtee/tmt#2221 - Fix test checking custom destination for libraries
    * teemtee/tmt#2216 - Create plans to cover individual step features
    * teemtee/tmt#2206 - Group discover/fmf options, improve wording a bit

Not sure how exactly the query author:{0}+merged:{1}..{2} works but definitely some of the pull requests merged by me during this week are not listed. See above.

2uasimojo · 2023-11-15T23:35:34Z

I'll say that I don't understand the logic behind "closed". When I use --github-pull-requests-closed the list doesn't seem to include any pull requests I authored. What I want to see (non-exclusively) is pull requests I worked on (authored or co-authored) that have merged. If this PR makes that possible, I'm all for it.

cardil · 2023-11-16T10:24:05Z

PR can be closed because of being merged or declined. This takes just the successful PRs.

psss · 2024-01-03T13:35:43Z

I'll say that I don't understand the logic behind "closed". When I use --github-pull-requests-closed the list doesn't seem to include any pull requests I authored. What I want to see (non-exclusively) is pull requests I worked on (authored or co-authored) that have merged. If this PR makes that possible, I'm all for it.

Pull requests authored by a user are covered by pull-requests-created. The intention behind the pull-requests-closed stat was to give the credit to the person who took care of merging the pull request. This can cover making sure that tests passed and whatever action or process is agreed in the project. Such person should be set as the pull request assignee. Do you propose a different approach?

PR can be closed because of being merged or declined. This takes just the successful PRs.

Understood. It might make sense to separate closed (without merge) and merged pull requests. However, we need to make sure the stat is working as expected. As mentioned in the comment above, some of the pull requests merged by me in the given time frame are not reported by the new stat. Could you please, have a look into that?

2uasimojo · 2024-01-03T15:38:49Z

Hi @psss!

Pull requests authored by a user are covered by pull-requests-created. The intention behind the pull-requests-closed stat was to give the credit to the person who took care of merging the pull request. This can cover making sure that tests passed and whatever action or process is agreed in the project. Such person should be set as the pull request assignee. Do you propose a different approach?

I think of the assignee as the reviewer*. I dig the idea of did having a knob to report all the PRs I reviewed. Useful subsets would be:

PR is still open (my "review queue"), possibly reporting how many reviews I've already left on it.
PR was closed because merged.
PR was closed without merge.

--pull-requests-created is really great... but it would be really useful to see a similar breakdown for those, which I think was the intention behind this PR.

Having worked on CLIs for more than 20y, I appreciate that you may not want to maintain a massive proliferation of flags. That said, I could envision the following non-mutually-exclusive options that could be used in conjunction with all/most of the --github-* ones:

--open (n/a -- or just a guaranteed empty set -- for --github-*-closed)
--merged
--abandoned

...where the default is --open --merged --abandoned

I haven't thought through whether/how these could also apply to --jira-*, but it could be similar.

*I imagine different shops use different processes, so this may not apply universally, but this is how it's done at Red Hat, at least in the OpenShift org, so I know it's not just me :)

psss · 2024-01-03T16:04:01Z

I think of the assignee as the reviewer*.

Well, for reviewers there's a dedicated field, right? I would not suggest to duplicate the meaning.

PR is still open (my "review queue"), possibly reporting how many reviews I've already left on it.

Just to clarify, the existing pull-requests-reviewed stat does not cover this use case? Unfortunately, the api for reviewed pull reuqests does not always produce correct results (reported the issue several years ago but no progress on that). So I'm not sure how much this can be improved.

PR was closed because merged.

+1 for having pull-requests-merged

PR was closed without merge.

Your suggested name pull-requests-abandoned makes it less ambiguous, so +1 for this as well.

2uasimojo · 2024-01-03T20:38:38Z

I think of the assignee as the reviewer*.

Well, for reviewers there's a dedicated field, right? I would not suggest to duplicate the meaning.

Mm, fair point. The difference being that any schmo can leave a review, whereas in our process the assignee's reviews are the "important" ones -- specifically the ones that the author expects to move the PR toward getting merged. I guess the subtleties of different processes come into play here... but empirically having separate flags for "assigned" vs "reviewed" PRs would give users the flexibility to report in whatever way is most appropriate for them.

PR is still open (my "review queue"), possibly reporting how many reviews I've already left on it.

Just to clarify, the existing pull-requests-reviewed stat does not cover this use case?

If I could further filter by open/abandoned/merged, I would think so, yes.

cardil · 2024-01-08T19:22:11Z

@psss As mentioned in the #311 (comment), some of the pull requests merged by me in the given time frame are not reported by the new stat. Could you please, have a look into that?

This is straightforward. The -merged is reporting only the PRs you have authored/created, and then it was merged.

The -closed also lists the PRs authored/created by other people, you have approved. That's different.

I need the merged PR (authored by me), as I report the code I, personally, contributed.

psss · 2024-01-08T19:40:30Z

I need the merged PR (authored by me), as I report the code I, personally, contributed.

Isn't the credit for authoring a pull request covered by pull-requests-created? What is the work you want to track if you don't review and don't merge the pull request? I would say the contribution of the author is to create the pull request.

cardil · 2024-01-09T10:29:38Z

@psss Isn't the credit for authoring a pull request covered by pull-requests-created? What is the work you want to track if you don't review and don't merge the pull request? I would say the contribution of the author is to create the pull request.

The created, might not be merged. So, created contain 3 types: ongoing PRs, successful, and dropped. I just want successful ones, which I've created - merged. Also, the time is important. Creation time != merge time. I want the merge time.

psss · 2024-10-04T16:29:46Z

Thanks much for the clarification and sorry for the late response. So it's about getting the work until the finish (merging) and the date is clearly different. Makes sense now. Let's do it as you suggest. Could you please rebase on the latest main?

sandrobonazzola · 2025-05-28T16:04:50Z

Can you please rebase?

cardil · 2025-11-17T18:08:15Z

@sandrobonazzola: Can you please rebase?

Done. Sorry for being inactive on this!

sandrobonazzola

The GitHub part looks good for merging. Some questions on the GitLab one.

Other questions:

Should we filter by merge date or update date? The PR discussion suggests merge date, but the implementation seems to use update date.
Should we add unit tests similar to the existing GitLab tests?

sandrobonazzola · 2025-11-18T07:48:26Z

did/plugins/gitlab.py

+            'author_username': username,
+            'state': state,
+            'updated_after': since,
+            'updated_before': until


Is it intentional not setting get_all_results=True here?
Only the first page of results will be fetched here.

Also, has the GitLab global merge_requests endpoint been tested with a real token? Does it work for all users or only admins?

Thanks for catching this! I forgot about pagination. I've fixed this by adding get_all_results=True to the get_user_mr() method call.

The /api/v4/merge_requests endpoint is accessible to any authenticated user and respects their visibility permissions - users will only see MRs they have access to view.

Yes, I tested this on a real Gitlab instance. In fact, I've been using this branch for years now! 😆

sandrobonazzola · 2025-11-18T07:53:09Z

did/plugins/gitlab.py

+        super().__init__(data, parent, set_id)
+
+    def _get_title(self):
+        return self.data['title']


isn't self.data['target_title'] available for MergeRequests?

IMHO, the MergedRequest class is necessary because of data structure differences between the global merge_requests API and the events API:

Global API (used by get_user_mr()): Returns MR objects directly with fields like iid and title

Events API (used by other MergeRequest stats): Returns event objects with nested fields like target_id and target_title

I've added detailed comments explaining this in the code. The MergedRequest class handles these differences by:

Taking iid directly from the data instead of making an extra API call

Using data['title'] instead of data['target_title']

Without this class, we'd need to modify the base Issue class to handle both data formats, which would complicate the code.

Take a look at 8f5e5ee. I found, that the Markdown formatting requires some params in data, so I remapped the param, and so the _get_title() isn't needed anymore.

bsipocz · 2025-11-18T11:57:04Z

In its current form this should close #371

cardil · 2025-11-18T12:07:45Z

@sandrobonazzola:

Should we filter by merge date or update date? The PR discussion suggests merge date, but the implementation seems to use update date.

That's right. I'll update the code to use the merge date. Those two dates are often the same, but the MR might be updated after it's merged, so yes.

Should we add unit tests similar to the existing GitLab tests?

Yep, I'll add those...

cardil · 2025-11-18T13:26:22Z

@cardil: I'll update the code to use the merge date.

@cardil: Yep, I'll add those...

Both fixed in dd7c2d5

cardil · 2025-11-19T12:08:23Z

The readthedocs #30370221 failure was likely due to yesterday's Github outage: https://www.githubstatus.com/incidents/5q7nmlxz30sk

Filter by merged_at timestamp (not updated_at) and add tests for both GitLab and GitHub merge-requests-merged functionality.

Transform MR data structure to include target_title and target_type fields expected by parent Issue class, eliminating the need for method overrides.

kwk · 2025-11-20T14:57:43Z

Maybe this was already asked. If I were to merge this very PR I suppose it will not show up in the list because I'm not the author, right? Is that what we want? I think there's value in both, PRs / MRs that were merged on your behalf or those that you've merged yourself.

cardil · 2025-11-20T15:17:53Z

Maybe this was already asked. If I were to merge this very PR I suppose it will not show up in the list because I'm not the author, right? Is that what we want? I think there's value in both, PRs / MRs that were merged on your behalf or those that you've merged yourself.

This PR is adding -merged flavor, which means: "You created the PR, and it was merged". Someone else might have authored the commits. It doesn't matter who clicks the merge button for this PR. PR author != commit author != commit commiter.

Usually in open-source you can't merge PRs yourself. And if you do, this PR is also the thing you want: the work you proposed, and then it was merged.

You want to list the PRs you merged, even though you are not the author of them? If so, this PR doesn't do that.

psss changed the title ~~[WIP] 💝 Listing Merged PRs for Github and Gitlab~~ Support merged pull requests for github and gitlab Oct 4, 2024

psss self-assigned this Oct 4, 2024

psss added plugin | gitlab plugin | github labels Oct 4, 2024

psss added this to the 0.22 milestone Oct 4, 2024

psss removed this from the 0.22 milestone Jun 3, 2025

psss mentioned this pull request Jun 19, 2025

BUG: --github-pull-requests-closed doesn't seem to work #371

Open

cardil force-pushed the feature/merged-pr branch from 857b9f6 to 2c698a5 Compare November 17, 2025 18:02

cardil marked this pull request as ready for review November 17, 2025 18:05

sandrobonazzola reviewed Nov 18, 2025

View reviewed changes

cardil force-pushed the feature/merged-pr branch from 2c698a5 to c048fbb Compare November 18, 2025 11:30

cardil force-pushed the feature/merged-pr branch from c048fbb to dd7c2d5 Compare November 18, 2025 13:21

Listing Merged PRs for Github and Gitlab

54823a3

cardil added 2 commits November 20, 2025 12:39

Fix merge date filtering and add unit tests

c8b868d

Filter by merged_at timestamp (not updated_at) and add tests for both GitLab and GitHub merge-requests-merged functionality.

Fix markdown formatting for merged requests

ae4b8d0

Transform MR data structure to include target_title and target_type fields expected by parent Issue class, eliminating the need for method overrides.

sandrobonazzola force-pushed the feature/merged-pr branch from 8f5e5ee to ae4b8d0 Compare November 20, 2025 11:39

sandrobonazzola assigned cardil and unassigned psss Nov 20, 2025

sandrobonazzola requested review from kwk, psss and sandrobonazzola November 20, 2025 11:40

Support merged pull requests for github and gitlab #311

Are you sure you want to change the base?

Support merged pull requests for github and gitlab #311

Conversation

cardil commented Jul 26, 2023

Changes

Uh oh!

psss commented Jul 27, 2023

Uh oh!

2uasimojo commented Nov 15, 2023

Uh oh!

cardil commented Nov 16, 2023

Uh oh!

psss commented Jan 3, 2024

Uh oh!

2uasimojo commented Jan 3, 2024

Uh oh!

psss commented Jan 3, 2024

Uh oh!

2uasimojo commented Jan 3, 2024

Uh oh!

cardil commented Jan 8, 2024

Uh oh!

psss commented Jan 8, 2024

Uh oh!

cardil commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psss commented Oct 4, 2024

Uh oh!

sandrobonazzola commented May 28, 2025

Uh oh!

cardil commented Nov 17, 2025

Uh oh!

sandrobonazzola left a comment

Choose a reason for hiding this comment

Uh oh!

sandrobonazzola Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

sandrobonazzola Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

cardil Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

sandrobonazzola Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

cardil Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

cardil Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Nov 18, 2025

Uh oh!

cardil commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cardil commented Nov 18, 2025

Uh oh!

cardil commented Nov 19, 2025

Uh oh!

kwk commented Nov 20, 2025

Uh oh!

cardil commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Support `merged` pull requests for `github` and `gitlab` #311

Support `merged` pull requests for `github` and `gitlab` #311

cardil commented Jan 9, 2024 •

edited

Loading

cardil Nov 18, 2025 •

edited

Loading

cardil commented Nov 18, 2025 •

edited

Loading

cardil commented Nov 20, 2025 •

edited

Loading