ondemand icon indicating copy to clipboard operation
ondemand copied to clipboard

JobComposer: Fix the way Manifests handle default host

Open ericfranz opened this issue 7 years ago • 5 comments

This pull request added a temporary fix to the issue: https://github.com/AweSim-OSC/osc-jobconstructor/pull/150

Loading the manifest should not result in a crash. A TODO comment was added to address this: https://github.com/AweSim-OSC/osc-jobconstructor/blob/a3c3d7b1c32e2f76dc9d6ad674483d87e940f5df/app/models/manifest.rb#L48-L50

The correct solution is 3 fold:

  1. OODClusters should be a Clusters instance, so you can just do OODClusters.first.id instead of OODClusters.first[0].to_s
  2. If there are no clusters configured OODClusters.count should return 0 and OODClusters.first should return nil. We shouldn't throw an exception; rather add a guard clause and return the appropriate value (nil or "" if no cluster is set).
  3. Manifest should probably not know about OODClusters. Rather, Manifest should probably have a class attribute default_host that is used, and we can set this in an initializer.

┆Issue is synchronized with this Asana task by Unito

ericfranz avatar Sep 07 '16 15:09 ericfranz

OODClusters is a global variable assigned in an app initializer.

brianmcmichael avatar Sep 07 '16 15:09 brianmcmichael

I think it's more important to provide a warning to the user if no clusters are configured, since the app will not be able to submit any jobs. This is more of an issue for the sysadmins than the user though.

Converting OODClusters to Clusters is reasonable going forward, but will require major overhaul to this app and Clusters is currently largely undocumented and in a state of developmental flux.

brianmcmichael avatar Sep 07 '16 15:09 brianmcmichael

I think it's more important to provide a warning to the user if no clusters are configured, since the app will not be able to submit any jobs.

Yes we should do that to, but that warning should be created in the initializer where OODClusters is first created. Thats no the domain of the Manifest

ericfranz avatar Sep 07 '16 15:09 ericfranz

Agreed.

brianmcmichael avatar Sep 07 '16 15:09 brianmcmichael

Converting OODClusters to Clusters is reasonable going forward, but will require major overhaul to this app and Clusters is currently largely undocumented and in a state of developmental flux.

Clusters won't be changing anytime soon - we might add methods to it but thats it. Its in a release of OodAppkit now. To make these changes we will need to identify all the places that use OODClusters and write tests that cover those places (at least one to cover them). That will make the refactor much safer and faster.

ericfranz avatar Sep 07 '16 15:09 ericfranz