ondemand
ondemand copied to clipboard
JobComposer: Fix the way Manifests handle default host
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:
- OODClusters should be a
Clusters
instance, so you can just doOODClusters.first.id
instead ofOODClusters.first[0].to_s
- If there are no clusters configured
OODClusters.count
should return 0 andOODClusters.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). - 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
OODClusters
is a global variable assigned in an app initializer.
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.
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
Agreed.
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.