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

spoolman: Include total count header #843

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

Clon1998
Copy link
Contributor

Closes #841.

As noted in the linked issue, spoolman uses the x-total-count header field in its response to indicate the total count, if appropriate, for a pagination result.

To also include this in the proxy I modified the returned JSON of the V2 response to not only return the response but also a response_header field with all custom headers (Headers starting with x-).

I am open to suggestions or alternative ideas on how to handle this change. I feel that adding it as part of the V2 is the safest and will not break any current implementations.

@Clon1998 Clon1998 changed the title [Spoolman proxy] Include total count header spoolman: Include total count header Apr 22, 2024
@Arksine
Copy link
Owner

Arksine commented Apr 24, 2024

I can see how this would be useful. I'm wondering if we should convert the full headers object to a dict and return them all. Are we sure we only need the extended headers? The entire headers object may not be necessary, but I don't think it would hurt to return them all either.

Also, when ready to merge, don't forget to add your signed off line. Thanks.

@Clon1998
Copy link
Contributor Author

I can see how this would be useful. I'm wondering if we should convert the full headers object to a dict and return them all. Are we sure we only need the extended headers? The entire headers object may not be necessary, but I don't think it would hurt to return them all either.

Also, when ready to merge, don't forget to add your signed off line. Thanks.

For now I just went with all custom headers, but as you mentioned including all headers in the result won't hurt either.
I'll adjust the change and also adapt the PR within your guidelines. Sorry about that.

Added the response headers to the V2 response of the proxy implementation as a new field named "response_headers".

Signed-off-by: Patrick Schmidt <[email protected]>
@Arksine Arksine merged commit c857e1a into Arksine:master Apr 24, 2024
1 check passed
@Arksine
Copy link
Owner

Arksine commented Apr 24, 2024

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spoolman Proxy: Include total count header in response
2 participants