Skip to content
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

api: GetPullRequestCommits: return file list #27483

Merged
merged 10 commits into from
Oct 9, 2023
Merged

Conversation

msantos
Copy link
Contributor

@msantos msantos commented Oct 6, 2023

Fixes #27481


Patch tested:

[
  {
    "url": "http://100.115.92.198:9292/api/v1/repos/msantos/test/git/commits/7664dcb44167e0f9efd994e4ca6a9164694adc27",
    "sha": "7664dcb44167e0f9efd994e4ca6a9164694adc27",
    "created": "2023-10-06T09:57:08-04:00",
    "html_url": "http://100.115.92.198:9292/msantos/test/commit/7664dcb44167e0f9efd994e4ca6a9164694adc27",
...
    "files": [
      {
        "filename": "README.md",
        "status": "modified"
      }
    ],
    "stats": {
      "total": 2,
      "additions": 2,
      "deletions": 0
    }
  }
]

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 6, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 6, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Oct 6, 2023
@silverwind
Copy link
Member

silverwind commented Oct 6, 2023

Was this a regression? files: null does indicate i may have been. If there are tests for this api call, I suggest validating presence of files.

@msantos
Copy link
Contributor Author

msantos commented Oct 6, 2023

Was this a regression? files: null does indicate i may have been. If there are tests for this api call, I suggest validating presence of files.

Yes, I run some status checks using files. I'd have to check but I think it was working in 1.19. I'll look into the tests.

@silverwind
Copy link
Member

Thanks, maybe you can also check the 1.20 and 1.21 branches so we know how far to backport this.

@silverwind silverwind added type/bug issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change labels Oct 6, 2023
@msantos
Copy link
Contributor Author

msantos commented Oct 6, 2023

Thanks, maybe you can also check the 1.20 and 1.21 branches so we know how far to backport this.

Introduced in 1dd83db released in 1.20.

I see the above commit made returning files/verification optional for the commit endpoint by using a parameter:

curl -X 'GET'   'https://try.gitea.io/api/v1/repos/msantos/test/commits?files=true'   -H 'accept: application/json'

The commit changed 3 endpoints: commit, pull and notes.

  • pulls don't include verification
  • notes don't include files or verification

Is it ok to have pulls always return files or should it be a parameter?

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 6, 2023
@silverwind silverwind added backport/v1.20 This PR should be backported to Gitea 1.20 backport/v1.21 This PR should be backported to Gitea 1.21 labels Oct 7, 2023
@lunny lunny added kind/bugfix and removed type/bug labels Oct 7, 2023
Update the pulls and notes endpoints:
* reenable returning files and verification
* optionally disable using a parameter
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 7, 2023
@msantos
Copy link
Contributor Author

msantos commented Oct 7, 2023

Updated to:

  • return files/verification fields by default in the pulls and notes endpoints
  • optionally disable using the fields and verification parameters
  • added tests

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 7, 2023
@delvh delvh added type/bug and removed kind/bugfix labels Oct 7, 2023
@delvh delvh removed the kind/api label Oct 7, 2023
Endpoint parameter docs updated using go-gitea@1dd83db
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 8, 2023
@silverwind silverwind removed backport/v1.20 This PR should be backported to Gitea 1.20 backport/v1.21 This PR should be backported to Gitea 1.21 issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change labels Oct 8, 2023
@silverwind
Copy link
Member

silverwind commented Oct 8, 2023

Given that previous files api was undocumented, I think we can strictly speaking not call it a regression, but I'd be willing to have it in 1.21.

@silverwind silverwind added the backport/v1.21 This PR should be backported to Gitea 1.21 label Oct 8, 2023
@delvh delvh removed the kind/api label Oct 8, 2023
@msantos
Copy link
Contributor Author

msantos commented Oct 8, 2023

Given that previous files api was undocumented, I think we can strictly speaking not call it a regression, but I'd be willing to have it in 1.21.

1dd83db added the new files api to the commits endpoint in 1.20 by modifying a function shared by the notes and pulls endpoints. The change unconditionally disabled returning values for the files and verification fields in the notes/pulls endpoints.

The change in this PR:

  • notes/pulls: restores the behaviour in 1.19 of returning files/verification
  • notes/pulls: allows optionally disabling files/verification by copying the files api from 1dd83db

The second item is unrelated to #27481 and could be dropped or split into a separate PR. Sorry for any confusion!

@wxiaoguang wxiaoguang enabled auto-merge (squash) October 9, 2023 04:36
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 9, 2023
@wxiaoguang wxiaoguang merged commit 5283ce9 into go-gitea:main Oct 9, 2023
24 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Oct 9, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 9, 2023
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Oct 9, 2023
silverwind added a commit that referenced this pull request Oct 9, 2023
Backport #27483 by @msantos

Fixes #27481

---
Patch tested:

```json
[
  {
    "url": "http://100.115.92.198:9292/api/v1/repos/msantos/test/git/commits/7664dcb44167e0f9efd994e4ca6a9164694adc27",
    "sha": "7664dcb44167e0f9efd994e4ca6a9164694adc27",
    "created": "2023-10-06T09:57:08-04:00",
    "html_url": "http://100.115.92.198:9292/msantos/test/commit/7664dcb44167e0f9efd994e4ca6a9164694adc27",
...
    "files": [
      {
        "filename": "README.md",
        "status": "modified"
      }
    ],
    "stats": {
      "total": 2,
      "additions": 2,
      "deletions": 0
    }
  }
]
```

Co-authored-by: Michael Santos <[email protected]>
Co-authored-by: silverwind <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 10, 2023
* giteaofficial/main:
  Respect SSH.KeygenPath option when calculating ssh key fingerprints (go-gitea#27536)
  Remove max-width and add hide text overflow (go-gitea#27359)
  Fix `environment-to-ini` inherited key bug (go-gitea#27543)
  Improve docs about register/run as a service (go-gitea#27461)
  api: GetPullRequestCommits: return file list (go-gitea#27483)
  switch to using official AWS step in release nightly (go-gitea#27532)
  Improve file history UI and fix URL escaping bug (go-gitea#27531)
  Improve dropdown's behavior when there is a search input in menu (go-gitea#27526)
  Simplify `contrib/backport` (go-gitea#27520)
  Add docs section for sub-paths with the container registry (go-gitea#27505)
  Document our new labeling strategy (go-gitea#27523)
  [skip ci] Updated translations via Crowdin
  Restore warning commit status (go-gitea#27504)
  Update labeler to match new labeling system (go-gitea#27525)
  Apply to become a maintainer (go-gitea#27522)
  Remove unnecessary desc for openssh key cron task (go-gitea#27515)
  Tweak labeler config (go-gitea#27502)
  Add hover background to wiki list page (go-gitea#27507)
  [FIX] missing ctx in new_form (go-gitea#27514)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

routers/api/v1/repo /pull.go: GetPullRequestCommits: empty Files list
7 participants