Bug #9687

[Workbench] A container_request should not be displayed as "successful" if the container exited non-zero.

Added by Radhika Chippada over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench
Target version:
Start date:
08/29/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5
Release:
Release relationship:
Auto


Subtasks

Task #9809: Review 9687-container-request-displayResolvedLucas Di Pentima

Associated revisions

Revision fbb97971
Added by Lucas Di Pentima over 4 years ago

Merge branch '9687-container-request-display'
Closes #9687

History

#1 Updated by Radhika Chippada over 4 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg over 4 years ago

If a container fails, we should have
  • ContainerRequest state: Committed
  • Container state: Complete
  • Container exit code: number != 0

In this example the container has

{
  "state": "Complete",
  "exit_code": 33
}

This state should be rendered in Workbench as a failure.

#3 Updated by Tom Clegg over 4 years ago

  • Subject changed from [Crunch2] The status of a container_request is is success even when the status of one of it's children is failed. to [Workbench] A container_request should not be displayed as "successful" if the container exited non-zero.
  • Category set to Workbench

#4 Updated by Tom Clegg over 4 years ago

  • Story points set to 0.5

#5 Updated by Tom Morris over 4 years ago

  • Assigned To set to Lucas Di Pentima
  • Target version set to 2016-08-31 sprint

#6 Updated by Lucas Di Pentima over 4 years ago

  • Status changed from New to In Progress

#7 Updated by Lucas Di Pentima over 4 years ago

Fixed the issue at the container_work_unit model. Added related fixture and test @ 74b5dd2

#8 Updated by Radhika Chippada over 4 years ago

74b5dd2b6

  • Can you please update the container_work_unit -> state_label method something as follows? Basically, the get_combined already gets the value from the “other” object as needed …
    exit_code = get_combined(:exit_code)
    return "Failed" if (exit_code && exit_code != 0)
    get_combined(:state)
  • Please consider adding “ exit_code: 0” to all other “Complete” container fixtures.
  • Doesn’t hurt to expand the test "state_label should be Failed if container exit_code not 0" to also check that it is “not Failed” when exit code is 0 (even though it is implicitly tested elsewhere)

#9 Updated by Lucas Di Pentima over 4 years ago

Above comments addressed @ 24eee13

#10 Updated by Radhika Chippada over 4 years ago

Lucas: Looking at the test, I can’t really tell what it’s purpose is. I think it can be a bit more self explanatory. Something as follows?

    …
  ].each do |cr_fixture, state, exit_code|
    test “Completed ContainerRequest state = #{state} when exit_code = #{exit_code}” do
       …
       assert_equal exit_code, …
       assert_equal state, …

Rest LGTM. Thanks.

#11 Updated by Lucas Di Pentima over 4 years ago

The test was written that way because I didn't want to add an exit_code accessor method just to support it, I now have updated the test as per your comments, but had to add that method to the model, you can check it at 8da182d .

#12 Updated by Radhika Chippada over 4 years ago

  • Lucas said: The test was written that way because I didn't want to add an exit_code accessor method

Yes, we need this. Also, please add "def exit_code" to work_unit.rb as well (this is to serve as the interface definition of all work units). We do not need to implement this method in all other work_unit typed classes; it will just default to null in the case of other object types.

  • Nit. Now that you have this method, you can use it in the state_label method rather than repeating the logic here
   def state_label
-    exit_code = get_combined(:exit_code)
+    ec = exit_code

Thanks. LGTM

#13 Updated by Lucas Di Pentima over 4 years ago

Merged master into this branch, resolved conflicts at 3678eda .
Running entire suite before merging into master.

#14 Updated by Lucas Di Pentima over 4 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:fbb979710d3cf5e2ee8c46936ac81081ef553b5c.

Also available in: Atom PDF