-
Notifications
You must be signed in to change notification settings - Fork 200
Package JSON API (replacement of #810) #996
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
Conversation
b7cebc6
to
72f1cdd
Compare
a261d24
to
3932f46
Compare
@Kleidukos why did you reformat the whole cabal file? It makes this rather messy. Are there any substantive changes to the cabal file, and if not, can you revert that portion? |
@gbaz nope, only minimal changes from the original PR, I'll revert the formatting changes. |
3932f46
to
9ea6c75
Compare
If you fix the failing tests I will merge. (I'm pretty sure the failure is the "bad package info" error, and the validation warnings can be disregarded). |
The test fails with:
Which means that version HTML endpoints that do not have the .html extension in the URL are actually serving JSON. |
@gbaz ping? |
Been caught up with other things. Taking a look now. |
The issue is not with the server, but with the test. It uses Network.HTTP to make a GET request, and that doesn't include a header specifying the content type, and the framework for some reason picks json instead of html when no content type is specified. There might be a fix to the framework to default the other way, but for the most part I think we can expect any modern client to specify a content type. |
If you try this branch with the changes in the above pr, tests should pass. If so, we can merge that branch, then we can rebase this over that and merge this. |
6f8406b
to
ed32ebe
Compare
This commit adds JSON endpoints for packages, and integrates commit 866f99d that defaults the Content-Type to text/html when negotiating content. Co-authored-by: Hécate Moonlight <[email protected]> Co-authored-by: Gershom Bazerman <[email protected]>
ed32ebe
to
aae6abc
Compare
@gbaz Thanks a lot for the help! It should be good to merge now. :) |
This PR breaks the
Revert? Test also broken locally on my machine (macOS):
|
Is this the failure you're talking about? |
@Kleidukos: Atm, I cannot reproduce the failure locally. Unfortunately, I have overwritten the logs with the failure by running the test again. But the CI logs are still present. E.g. I copied an error from the log of run 1 I quote in #996 (comment):
I see now that the same failure occurred on: https://github.com/haskell/hackage-server/runs/5267206134?check_suite_focus=true which was before your PR. It seems the test fails now and then. Cf. runs on pushes to |
This PR finishes the job of #810.