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

Fix #1512: Plugin doesn't abort building an image in case Podman is u… #1585

Merged
merged 2 commits into from
Jul 31, 2022
Merged

Fix #1512: Plugin doesn't abort building an image in case Podman is u… #1585

merged 2 commits into from
Jul 31, 2022

Conversation

jh-cd
Copy link
Contributor

@jh-cd jh-cd commented Jul 21, 2022

Fix #1512

In the previous version of this file the following if() was as follows. The methode isJson() checks by header (not by body):

     if(isJson(response)) {
          EntityStreamReaderUtil.processJsonStream(handler, stream);
     }

In case the Podman daemon is used, the POST /build response is JSON and the HTTP status code is 200
as expected, but there is no header "Content-Type = application/json" nor "Content-Type" at all. Seen in
Podman 3.4.2. But the docker-maven-plugin relies on that JSON-body. In BuildJsonResponseHandler.process():

     if (json.has("error")) {
    ...
    throw new DockerAccessException();

If no error is detected, the Maven-build goes on despite there was a problem building the
image!
The following if() first checks for the application/json Content-Type. If no Content-Type is set,
it tries to detect if the body is JSON. If so, the handler is called.

…sed and the Dockerfile can't be processed

Signed-off-by: Johannes Heger <[email protected]>
@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Merging #1585 (88b2da5) into master (df4d7d0) will increase coverage by 0.03%.
The diff coverage is 85.00%.

@@             Coverage Diff              @@
##             master    #1585      +/-   ##
============================================
+ Coverage     63.51%   63.54%   +0.03%     
- Complexity     2163     2169       +6     
============================================
  Files           170      170              
  Lines          9905     9923      +18     
  Branches       1359     1360       +1     
============================================
+ Hits           6291     6306      +15     
- Misses         3085     3087       +2     
- Partials        529      530       +1     
Impacted Files Coverage Δ
...ker/access/hc/HcChunkedResponseHandlerWrapper.java 82.75% <85.00%> (+0.94%) ⬆️

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

89.3% 89.3% Coverage
0.0% 0.0% Duplication

@manusa manusa self-requested a review July 27, 2022 14:25
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

Logic seems fine.

Maybe you'd want to introduce a complete test case with JSON response that verifies the scenario described by the issue.

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.

Plugin doesn't abort building an image in case Podman is used and the Dockerfile can't be processed
3 participants