Add Accept header #954
Add Accept header #954
Conversation
|
Build failed. ❌ pre-commit NODE_FAILURE Node request 200-0008095228 failed in 0s |
Summary of ChangesHello @majamassarini, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where Fedora infrastructure was misinterpreting requests from Packit, leading to incorrect responses. By introducing explicit 'Accept' headers in HTTP requests to the Pagure API, the system can now correctly identify the expected content type, ensuring proper communication and preventing misidentification as a generic bot. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the ability to specify an Accept header for requests to Pagure, with the goal of correctly fetching YAML files from Fedora infrastructure. A default Accept: application/json header is added for API calls, and get_file_content is modified to request application/yaml.
My review found a critical issue in the implementation of header merging which would break all authenticated API calls. I've provided a suggestion to fix this. I also have a medium-severity comment about a potential design issue where a generic method is made specific, which could affect maintainability.
majamassarini
force-pushed
the
add-accept-header
branch
from
3aa491e to
430036f
Compare
|
Build failed. ❌ pre-commit NODE_FAILURE Node request 200-0008095230 failed in 0s |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds an Accept header to requests to Pagure to address an issue with Fedora infrastructure. The implementation correctly adds a default Accept: application/json header for API calls and allows overriding it, for example to application/yaml when fetching files. My review focuses on improving the robustness of the header handling logic and addressing a security concern with logging sensitive information. I've identified a few areas for improvement:
- The logic for providing default or merged headers can be made more robust by checking for
Noneinstead of relying on the truthiness of dictionaries. - A generic method is being specialized for a specific use case, which could impact maintainability.
- A new debug log statement is leaking authentication tokens, which is a security risk.
|
Build failed. ❌ pre-commit NODE_FAILURE Node request 200-0008095242 failed in 0s |
|
Build failed. ❌ pre-commit NODE_FAILURE Node request 200-0008095244 failed in 0s |
|
Build failed. ❌ pre-commit NODE_FAILURE Node request 200-0008095247 failed in 0s |
majamassarini
force-pushed
the
add-accept-header
branch
2 times, most recently
from
0e2d156 to
7ad5d57
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds an Accept header to requests made to Pagure to ensure the correct content type is returned, which is a good improvement for robustness. A service-wide default of Accept: application/json is set, and this is correctly overridden to text/plain for raw file content requests. The changes correctly propagate the header parameter through the call stack. I've found one logical issue in how headers are handled in get_file_content when custom headers are provided, and I've included a suggestion to fix it. Otherwise, the changes are well-implemented.
majamassarini
force-pushed
the
add-accept-header
branch
from
aa4967c to
4adc2e1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for custom request headers, specifically setting a default Accept: application/json header for Pagure API calls and Accept: text/plain for raw file content requests. This is to address an issue with Fedora infrastructure. The changes are well-implemented by adding an optional header parameter to the relevant API call methods and correctly merging service-level and call-specific headers. I've added a suggestion to improve type hinting for better code clarity and consistency.
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 03s |
majamassarini
force-pushed
the
add-accept-header
branch
from
2af88d6 to
f63758a
Compare
|
merge before packit/packit#2663 |
majamassarini
force-pushed
the
add-accept-header
branch
from
f63758a to
956c286
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds the ability to specify an Accept header in requests, which is needed for Fedora infrastructure. The default Accept header is now set to application/json for API calls, and text/plain for file content retrieval. The mechanism for handling custom headers has also been improved.
The implementation is solid, but I've left a couple of comments with suggestions for improvement:
- One regarding missing type hints and API consistency in
get_file_content. - Another to improve the accuracy of a new debug log message for request headers.
Overall, good work on addressing the infrastructure requirement.
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 58s |
majamassarini
force-pushed
the
add-accept-header
branch
from
d93e9b8 to
204ac86
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 03s |
majamassarini
force-pushed
the
add-accept-header
branch
from
204ac86 to
da257f2
Compare
Fedora infra needs it (otherwise packit is treated like any other AI bot and it is given back the Anubis page). Co-authored-by: Nikola Forró <[email protected]>
Co-authored-by: Nikola Forró <[email protected]>
majamassarini
force-pushed
the
add-accept-header
branch
from
61c3d8d to
7a1cbe9
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 56s |
|
/packit retest-failed |
3 participants
Fedora infra needs it (otherwise packit is treated like any other AI bot and it is given back the Anubis page).