pagure

#3857 Add a new API endpoint to retrieve the list of files changed in a PR
Merged by pingou. Opened by pingou.
api_pr_files_changed  into  master

Download 3857.patch

This new API endpoint allows to retrieve the list of files changed as
well as the number of lines added/removed and whether the file was added
removed, modified or renamed (cf the status field).

Fixes https://pagure.io/pagure/issue/3686

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

Can you rebase this and add tests?

@pingou this is duplicating a lot of logic that we currently have in the templates _repo_renderdiff.html

Would it be a good idea to abstract some of this out so we can use it in both places? I'd love to not have the pygit version checking craziness in that template :)

@ryanlerch that's exactly where I took the logic from :-p

big +1 to try to re-use this somehow. Should we plan for this in 5.2?

Actually, I look up the logic in repo_pull_request.html does that mean we have it in three places?

Can you rebase this and add tests?

That was the idea, but I pinged @fbo on IRC to ask him if he has enough information with this or if he needs more

rebased onto 50caff497ec58a9954ff498c4fb351b72bd5f659

There are several test failures:

14:10:18 FAILED test: py-test_pagure_flask_ui_old_commit
14:10:18 FAILED test: py-test_style
14:10:18 FAILED test: py-test_pagure_flask_ui_repo
14:10:18 FAILED test: py-test_pagure_flask_internal
14:10:18 FAILED test: py-test_pagure_lib_git
14:10:18 FAILED test: py-test_pagure_lib
14:10:18 FAILED test: py-test_pagure_flask_api

rebased onto 0a6c36891d2f71805bc0796b73abfdea9244bf7b

rebased onto efe89b6daa237f5671b1d69670eced170a68c3e0

rebased onto 89e9387cbc9165964c18af973bc3eca6e5e14579

7 new commits added

  • Debug things
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

8 new commits added

  • Fix the tests
  • Debug things
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

9 new commits added

  • fixup tests api
  • Fixup a logging call
  • Debug things
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

I was traveling today. So yes looking at the response sample that seems perfect. With that new feature Zuul will be able via the API to detect if a file named .zuul.yaml or zuul.d/*.yaml have changed :).

9 new commits added

  • fixup tests api
  • Fixup a logging call
  • Debug things
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

rebased onto 1dbff6fa229edfd99546a154c358c0062c8931c3

8 new commits added

  • Fixup a logging call
  • Debug things
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

@pingou Can you clean up the commits for merge? Aside from that, the code looks good to me.

7 new commits added

  • Fixup a logging call
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

7 new commits added

  • Fixup a logging call
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

A single failed test:

23:32:13 Failed tests:
23:32:13 FAILED test: py-test_pagure_flask_api_fork

Actually, I look up the logic in repo_pull_request.html does that mean we have it in three places?

Yes :/

Once for the changes summary in the comments tab, and once for the actual diff view as well. This is the way it always has been :/

1 new commit added

  • debug

1 new commit added

  • fixup test

7 new commits added

  • Fixup a logging call
  • Flake8 fixes
  • Adjust logged messages to fit the actual module file/name
  • Fix quotes in strings (left over from a time we didn't run black)
  • Add a new API endpoint to retrieve the list of files changed in a PR
  • Add an utility method to remove a file from a given git repo in the tests
  • Ensure two utility methods in the tests respect the branch they are given

rebased onto 6fdafd0b79a6883cc61b3078bd5c57cbe2de4d4b

The following block of code is repeated 3 times, maybe it could be refactored to as a function to avoid code duplication.

I agree with @ryanlerch, I think it would be nice to create functions in the lib/git.py maybe that takes care of the low level logic so these function can then just be reused in the templates and the API.

:thumbsup: with a ticket to refactor this so we don't forget

I agree with @ryanlerch, I think it would be nice to create functions in the lib/git.py maybe that takes care of the low level logic so these function can then just be reused in the templates and the API.
👍 with a ticket to refactor this so we don't forget

Thanks for the review, I opened the ticket at : https://pagure.io/pagure/issue/3868

I'm also looking at the refactoring of the redundant code in the tests, got something done and tests are running locally, if they pass I'll push and if jenkins is happy I'll merge :)

To give you an idea, these are the stats:

 tests/__init__.py | 108 ++++++++++++++++++++------------------------------------------------------------------------
 1 file changed, 23 insertions(+), 85 deletions(-)

1 new commit added

  • Refactor redundant code in the tests

Let's get this in \ó/

Pull-Request has been merged by pingou
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.