Arvados

Should also include satisfying defaults such as RAM and CPUs.

8470-resolve-container-req @ be4ca15

container_request.rb

• In resolve method
  • the comment “TODO: resolve container_image to a content address.” seems to have been addressed in container_image_for_container method?
  • If it is not too much to ask, in “Container.create!(command: self.command,” can you place all self.X items first and then the derived ones to improve readability of code?

• In mounts_for_container, line 139, can we say “mount #{uuid} not found” instead of “collection #{uuid} …”? Also, “uuid #{uuid} has portable_data_hash …”, can you say “mount uuid #{uuid} has …”?

container_request_test.rb

• In ‘test "Container request max priority" do’ , please add a comment explaining why cr2 priority is expected.

• In ‘test “Container makes container request, then is cancelled" do’ :
  • Add an assertion to check priority on cr after line 223 (after cancel)
  • I think you can use create_minimal_req! for the first cr creation
  • Can you please add a comment in this or in test “container cancelled finalizes request” or in code listing canceling what cancels what. I think a comment listing the relationships between Cs and CRs and Requesting_Cs and Container_uuids would be quite helpful.

• “resolve runtime constraint range #{mounts} to values” => mounts is not from runtime constraints, correct? should the name of this test be something like “resolve mounts to values …”?

Radhika Chippada wrote:

• the comment “TODO: resolve container_image to a content address.” seems to have been addressed in container_image_for_container method?

indeed. removed.

• If it is not too much to ask, in “Container.create!(command: self.command,” can you place all self.X items first and then the derived ones to improve readability of code?

done

• In mounts_for_container, line 139, can we say “mount #{uuid} not found” instead of “collection #{uuid} …”? Also, “uuid #{uuid} has portable_data_hash …”, can you say “mount uuid #{uuid} has …”?

improved error messages

• In ‘test "Container request max priority" do’ , please add a comment explaining why cr2 priority is expected.

done

• In ‘test “Container makes container request, then is cancelled" do’ :
  • Add an assertion to check priority on cr after line 223 (after cancel)
  • I think you can use create_minimal_req! for the first cr creation
  • Can you please add a comment in this or in test “container cancelled finalizes request” or in code listing canceling what cancels what. I think a comment listing the relationships between Cs and CRs and Requesting_Cs and Container_uuids would be quite helpful.

fixed up some comments here

• “resolve runtime constraint range #{mounts} to values” => mounts is not from runtime constraints, correct? should the name of this test be something like “resolve mounts to values …”?

whoops, yes, fixed... thanks