Bug 56418 - Manager incorrectly reports deployment as OK even though it failed
Manager incorrectly reports deployment as OK even though it failed
Status: RESOLVED FIXED
Product: Tomcat 7
Classification: Unclassified
Component: Manager
7.0.53
All All
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2014-04-16 20:59 UTC by Sylvain Laurent
Modified: 2014-04-25 16:23 UTC (History)
0 users



Attachments
Proposed patch on tomcat7 trunk (3.22 KB, patch)
2014-04-16 20:59 UTC, Sylvain Laurent
Details | Diff
Proposed patch on tomcat7 trunk (3.37 KB, patch)
2014-04-23 19:44 UTC, Sylvain Laurent
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sylvain Laurent 2014-04-16 20:59:13 UTC
Created attachment 31536 [details]
Proposed patch on tomcat7 trunk

When using the Manager to deploy a webapp remotely (uploading the war with an http PUT), the deployment status is incorrectly reported.
It does not occur when deploying a WAR that is on the server filesystem.

The proposed patch factors the check for the context state.
Comment 1 Christopher Schultz 2014-04-18 19:19:35 UTC
Comment on attachment 31536 [details]
Proposed patch on tomcat7 trunk

The first and second chunks of your patch change behavior (by adding an isAvailable check). Is that the real fix for this bug?

In general, I'm not a fan of having a "check" method dump anything to the output writer. Would it be possible to re-write the patch with the re-factored method but not dump any output? Or, maybe just change the name so it's clear that stuff will be printed to the output writer (even though it should be obvious to the reader than the writer is bring used since it's passed as a parameter).
Comment 2 Sylvain Laurent 2014-04-23 19:44:39 UTC
Created attachment 31554 [details]
Proposed patch on tomcat7 trunk

(In reply to Christopher Schultz from comment #1)
> The first and second chunks of your patch change behavior (by adding an
> isAvailable check). Is that the real fix for this bug?

Yes, exactly.
> 
> In general, I'm not a fan of having a "check" method dump anything to the
> output writer. Would it be possible to re-write the patch with the
> re-factored method but not dump any output? Or, maybe just change the name
> so it's clear that stuff will be printed to the output writer (even though
> it should be obvious to the reader than the writer is bring used since it's
> passed as a parameter).

OK, I renamed the method to "checkAndOutputContextDeploymentStatus", see new patch attached.

If this is OK, I can commit it since I should still have commit rights.
But I did not follow much tomcat dev lately : should I commit on 8.x  then 7.x ? should I update the changelog in the same commit or different one ?
Or if you prefer to commit this yourself, be my guest ;-)
Comment 3 Mark Thomas 2014-04-25 16:11:34 UTC
(In reply to Sylvain Laurent from comment #2)
> OK, I renamed the method to "checkAndOutputContextDeploymentStatus", see new
> patch attached.

I'm all for clear method names but that seems a little long

outputDeploymentResult()

should do.

> If this is OK, I can commit it since I should still have commit rights.
> But I did not follow much tomcat dev lately : should I commit on 8.x  then
> 7.x ?

Yes. Commit to 8.0.x and then svn merge back to 7.0.x.
 
> Should I update the changelog in the same commit or different one ?

Ideally the same one.

> Or if you prefer to commit this yourself, be my guest ;-)

As I am looking at this now I'll do it (but you will get the credit/blame in the changelog).
Comment 4 Mark Thomas 2014-04-25 16:23:06 UTC
Thanks.

Applied to 8.0.x for 8.0.6 and 7.0.x for 7.0.54.