Skip to content

Conversation

thaJeztah
Copy link
Member

integration-cli: fix getTestTokenService not sending header

This utility was setting the content-type header after WriteHeader was
called, and the header was not sent because of that.

distribution: TestPullSchema2Config fix test response

The test was depending on the client constructing an error based on the
http-status code, and the client not reading the response body if the
response was not a JSON response.

This fix;

  • adds the correct content-type headers in the response
  • includes error-messages in the response
  • adds additional tests to cover both the plain (non-JSON) and JSON
    error responses, as well as an empty response.

- A picture of a cute animal (not mandatory but encouraged)

This utility was setting the content-type header after WriteHeader was
called, and the header was not sent because of that.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The test was depending on the client constructing an error based on the
http-status code, and the client not reading the response body if the
response was not a JSON response.

This fix;

- adds the correct content-type headers in the response
- includes error-messages in the response
- adds additional tests to cover both the plain (non-JSON) and JSON
  error responses, as well as an empty response.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
{
name: "unauthorized JSON no body",
handler: func(callCount int, w http.ResponseWriter) {
w.Header().Set("Content-Type", "application/json")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do some registries really respond with application/json and an empty body? Because empty is not valid JSON, it needs to be [] or {}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think they would, or at least I would expect any server to have a body (which could be HTML), but the existing test seemed to handle it on the client side (I was actually surprised by that), so wanted to keep the test-case for that.

Copy link
Member

@rumpl rumpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question but LGTM

@thaJeztah thaJeztah added this to the 25.0.0 milestone Sep 27, 2023
@thaJeztah thaJeztah merged commit a1d966c into moby:master Sep 27, 2023
@thaJeztah thaJeztah deleted the distribution_test_fixes branch September 27, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants