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

Set the content type header on build #563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ekohl
Copy link

@ekohl ekohl commented Oct 25, 2020

Prior to this patch, the application/json Content-Type header was set but he body is a tar file as returned by Docker::Util.create_tar. That this code was there for a long time suggests Docker doesn't check the content type. However, podman's REST API does check the header and rejects the request. After this patch, it is accepted.

Prior to this patch, the application/json Content-Type header was set
but he body is a tar file as returned by Docker::Util.create_tar. That
this code was there for a long time suggests Docker doesn't check the
content type. However, podman's REST API does check the header and
rejects the request. After this patch, it is accepted.
@ekohl
Copy link
Author

ekohl commented Oct 26, 2020

It appears to fail on a lack of test coverage? Not sure how to solve that.

@trevor-vaughan
Copy link
Contributor

@ekohl Sorry, I didn't see this before going bananas on #569. Are you OK closing this in favor of that PR?

@ekohl
Copy link
Author

ekohl commented Feb 3, 2021

I think this is still correct. The content type is always application/json and that should be set. Retrying on a situation we already know exists is inefficient. So IMHO this should be merged. I don't like your solution to be honest.

@trevor-vaughan
Copy link
Contributor

@ekohl If you try more cases, you'll find that application/json is not always the content type. I did what you did at the first go but it rapidly failed when trying other operations. The "correct" fix is to have each operation actually pass what it needs but that will take more time than I have at the moment (particularly if they're not taking patches).

@ekohl
Copy link
Author

ekohl commented Feb 3, 2021

I agree. Given this hasn't seen a review from a maintainer in a few months I must admit I mostly forgot about this.

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.

2 participants