robur/builder-web
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Add Json API to some endpoints #5

Merged
reynir merged 10 commits from json_responses into main 2024-12-20 11:46:47 +00:00
Conversation 2 Commits 10 Files changed 8 +185 -30
PixieDust commented 2024-12-20 07:26:38 +00:00
Owner
Copy link

This PR uses the Accept: application/json header to return json responses for some endpoints:

  • /compare/:build_left/:build_right
  • /job/:job/build/latest/**
  • /job/:job/build/latest
This PR uses the `Accept: application/json` header to return json responses for some endpoints: - `/compare/:build_left/:build_right` - `/job/:job/build/latest/**` - `/job/:job/build/latest`
PixieDust added 4 commits 2024-12-20 07:26:38 +00:00
PixieDust requested review from hannes 2024-12-20 07:26:53 +00:00
PixieDust requested review from reynir 2024-12-20 07:26:54 +00:00
PixieDust added 1 commit 2024-12-20 07:29:31 +00:00
reynir reviewed 2024-12-20 08:32:48 +00:00
lib/builder_web.ml Outdated
@ -336,0 +337,4 @@
"uuid", `String (Uuidm.to_string build);
] |> Yojson.Basic.to_string
in
Dream.json ~status:`OK json_response |> Lwt_result.ok
reynir commented 2024-12-20 08:22:13 +00:00
Owner
Copy link

Tbh here I think I would just keep the redirect and rely on job_build returning a json object that includes the uuid.

Tbh here I think I would just keep the redirect and rely on `job_build` returning a json object that includes the uuid.
reynir commented 2024-12-20 08:24:03 +00:00
Owner
Copy link

The client would then need to follow redirects, of course.

The client would then need to follow redirects, of course.
PixieDust marked this conversation as resolved
lib/builder_web.ml
@ -411,0 +413,4 @@
"job_name", `String job_name;
"uuid", `String (Uuidm.to_string build.uuid);
"platform", `String build.platform;
"build_start_time", `String (Ptime.to_rfc3339 build.start);
reynir commented 2024-12-20 08:25:23 +00:00
Owner
Copy link

For consistency maybe drop the build_ prefix? Else I guess we should write build_uuid as well? :-)

For consistency maybe drop the `build_` prefix? Else I guess we should write `build_uuid` as well? :-)
PixieDust marked this conversation as resolved
lib/builder_web.ml Outdated
@ -541,0 +569,4 @@
} in
match id_opt with
| None -> Lwt.return_ok default_file
| Some id -> Model.build_artifact_by_id id conn
reynir commented 2024-12-20 08:29:35 +00:00
Owner
Copy link

I don't like that we make up a main binary if it doesn't exist. Can we use an option? For example, "0" is not a valid sha256 checksum, -1 is not a valid size...

I don't like that we make up a main binary if it doesn't exist. Can we use an option? For example, "0" is not a valid sha256 checksum, `-1` is not a valid size...
reynir commented 2024-12-20 08:35:54 +00:00
Owner
Copy link

Also, I think this function is only used in order to get the binary size? If so, then we can return that directly (as an option)

Also, I think this function is only used in order to get the binary size? If so, then we can return that directly (as an option)
PixieDust marked this conversation as resolved
lib/builder_web.ml Outdated
@ -584,0 +636,4 @@
"platform", `String build_right.platform;
"build_start_time", `String (Ptime.to_rfc3339 build_right.start);
"build_finish_time", `String (Ptime.to_rfc3339 build_right.finish);
"build_size", `Int (build_right_file.size)
reynir commented 2024-12-20 08:30:40 +00:00
Owner
Copy link

Then here we could make build_size optional or have it be `Null if the file is None.

Then here we could make `build_size` optional or have it be `` `Null `` if the file is `None`.
PixieDust marked this conversation as resolved
reynir added 4 commits 2024-12-20 09:30:02 +00:00 
Also add "main_binary" boolean field in comparison for consistency with
`job_build` json.
reynir commented 2024-12-20 09:32:14 +00:00
Owner
Copy link

I pushed a few commits that address my own comments.

I also tweaked a bit the json fields. In particular, I dropped the build_ prefix of the fields and tried to make the field names and members more consistent between the endpoints. The compare_builds json output now also has a "main_binary": true field even if it's somewhat redundant given the main binary size can tell you that.

@PixieDust: Please let me know if you agree and are happy with the changes

I pushed a few commits that address my own comments. I also tweaked a bit the json fields. In particular, I dropped the `build_` prefix of the fields and tried to make the field names and members more consistent between the endpoints. The compare_builds json output now also has a `"main_binary": true` field even if it's somewhat redundant given the main binary size can tell you that. @PixieDust: Please let me know if you agree and are happy with the changes
🚀 1
PixieDust commented 2024-12-20 09:38:05 +00:00
Author
Owner
Copy link

I pushed a few commits that address my own comments.

I also tweaked a bit the json fields. In particular, I dropped the build_ prefix of the fields and tried to make the field names and members more consistent between the endpoints. The compare_builds json output now also has a "main_binary": true field even if it's somewhat redundant given the main binary size can tell you that.

@PixieDust: Please let me know if you agree and are happy with the changes

Thank you @reynir . These changes are great :)

> I pushed a few commits that address my own comments. > > I also tweaked a bit the json fields. In particular, I dropped the `build_` prefix of the fields and tried to make the field names and members more consistent between the endpoints. The compare_builds json output now also has a `"main_binary": true` field even if it's somewhat redundant given the main binary size can tell you that. > > @PixieDust: Please let me know if you agree and are happy with the changes Thank you @reynir . These changes are great :)
reynir added 1 commit 2024-12-20 09:47:18 +00:00 
Now `or_error_response` will return the error message as a json object
if Accept: application/json.
reynir reviewed 2024-12-20 09:52:06 +00:00
lib/builder_web.ml
@ -77,1 +83,3 @@
| Error (text, status) -> Dream.respond ~status text
| Error (text, status) ->
if is_accept_json req then
let json_response = Yojson.Basic.to_string (`Assoc [ "error", `String text ]) in
reynir commented 2024-12-20 09:52:06 +00:00
Owner
Copy link

This could certainly be made into a much more complicated object. I don't have strong opinions on that.

This could certainly be made into a much more complicated object. I don't have strong opinions on that.
reynir commented 2024-12-20 09:54:31 +00:00
Owner
Copy link

There are "standards" out there suggesting you use an "errors" field with an array of error objects: https://jsonapi.org/format/#error-objects

There are "standards" out there suggesting you use an "errors" field with an array of error objects: https://jsonapi.org/format/#error-objects
👍 1
PixieDust commented 2024-12-20 10:37:09 +00:00
Author
Owner
Copy link

Yes that is probably the right way to go

Yes that is probably the right way to go
reynir commented 2024-12-20 11:48:31 +00:00
Owner
Copy link

I merged it as is. We can revisit the errors later.

I merged it as is. We can revisit the errors later.
reynir approved these changes 2024-12-20 10:15:07 +00:00
reynir merged commit b22f571a92 into main 2024-12-20 11:46:47 +00:00
reynir deleted branch json_responses 2024-12-20 11:46:47 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
hannes
reynir
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: robur/builder-web#5
No description provided.
Delete branch "json_responses"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?