search

VUIIS / dax

Distributed Automation for XNAT
MIT License
25 stars 24 forks source link

Update help string for --restart to make clear it applies to assessors of a given type. #393

Closed MikeDPhillips closed 2 years ago

MikeDPhillips commented 2 years ago

…present.

Bug described at: http://xnatbug.vandyxnat.org/bugzilla/show_bug.cgi?id=2501

Notes: Current change allows user to restart processors matching any former_status, not limited to JOB_FAILED.

Updated help string; current copy: Restart the assessors by switching the status for all assessors found to NEED_TO_RUN and delete previous resources.

baxpr commented 2 years ago

There is a lot of additional logic that uses args.rerundiskq. Is any of that going to be relevant/needed now with the change for args.restart?

I mean, does args.restart need its own set of that stuff

MikeDPhillips commented 2 years ago

There is a lot of additional logic that uses args.rerundiskq. Is any of that going to be relevant/needed now with the change for args.restart?

I mean, does args.restart need its own set of that stuff

That's a good question. I have not fully tested restart yet and I will check all the args and see what is still relevant or needs its own.

Also something I wanted to ask. If you look at the code I removed for restart I dont think this is an oversight or mistake it feels like a decision someone made for restart to work this way. So can you think of any edge case or something that this change might interfere with?

baxpr commented 2 years ago

Yeah, I think you're right. I don't think any of us are using it that way, but it was clearly intentional. Now I'm leaning toward leaving the code alone and just updating the help text to clarify that "restart" option ignores "former status"

MikeDPhillips commented 2 years ago

Yeah, I think you're right. I don't think any of us are using it that way, but it was clearly intentional. Now I'm leaning toward leaving the code alone and just updating the help text to clarify that "restart" option ignores "former status"

Nothing much to review here, I just changed the copy for the help text. I could also specifically write that it ignores the former_status flag.

baxpr commented 2 years ago

@MikeDPhillips let's have more clarity and say "restart all assessors regardless of status" or something.

Then this PR can just hang out until @bud42 is ready to grab it for a release.