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 for Remine ticket #10927: the server processes files are deleted only if the pipeline's exit code is 0. #2252

Merged
merged 2 commits into from
Oct 14, 2016

Conversation

nicolasbrossard
Copy link
Contributor

This should facilitate debugging in some cases.

@nicolasbrossard nicolasbrossard added the Feature PR or issue introducing/requiring at least one new feature label Oct 4, 2016
@nicolasbrossard nicolasbrossard added this to the 17.0 milestone Oct 4, 2016
*/
public function deleteProcessFiles()
{
return !is_null($exitCode) && is_numeric($exitCode) && $exitCode == 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

$exitCode doesn't seem to be defined in any scope that would mean the variable could be used here like this. It's neither an argument nor local variable..

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicolasbrossard @driusan
shouldn't the change have been made in:
https://github.com/aces/Loris/blob/master/modules/server_processes_manager/php/AbstractServerProcess.class.inc#L521
This is the line that I used to alternate between True/False depending on whether I wanted to keep those files or not...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleting the process files only if the exit code is 0 is something that is specific to the MRI upload processes. I think the default behaviour (to delete all files all the time, which is defined in AbstractServerProcess) should remain as is and should only be overridden in MriUploadServerProcess.

@alisterdev alisterdev added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Oct 7, 2016
@alisterdev alisterdev removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Oct 7, 2016
@MounaSafiHarab MounaSafiHarab added the Passed Manual Tests PR has undergone proper testing by at least one peer label Oct 11, 2016
@driusan driusan added the Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties label Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties Feature PR or issue introducing/requiring at least one new feature Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants