Add cluster batch_connect_ssh_allow? function to allow easier access to a new setting

[treydock]

Add cluster batch_connect_ssh_allow? function to allow easier access to a new setting:

batch_connect:
  ssh_allow: <boolean>

Needed for OSC/ondemand#1157

Not sure if this is best approach or if better to just add all the logic to OnDemand dashboard app to look for a certain config inside the cluster batch_connect settings.

[johrstrom]

Yea I went looking for the ticket, couldn't find it (if it ever existed), but someone from discourse wanted it to be per-job level too.

[johrstrom]

Sorry, not per job, I meant per app.

[treydock]

Could always chain things so that to show the link ENV['OOD_BC_SSH_TO_COMPUTE_NODE'] and this cluster level setting and app level setting must all be true, and if any of them are false, the SSH link is not rendered. They would all continue to default to true. If we do the chain then I think this change is needed plus one more for the app-level change then rest are inside dashboard.

[treydock]

@johrstrom Is this approach the right approach? I was thinking if this is merged then OnDemand for the helper generating SSH link could do something like this:

if ::Configuration.ood_bc_ssh_to_compute_node && session.cluster.batch_connect_ssh_allow? && session.ssh_allow?

Here: https://github.com/OSC/ondemand/blob/master/apps/dashboard/app/helpers/batch_connect/sessions_helper.rb#L98

If everything defaults to true then it would be an opt-out type thing but could be opt out at multiple levels.

[johrstrom]

I'm not entirely sure. I think it's right to have it at global/cluster/app levels. It's just about where to put it.

I think this could be the right place, I just don't know what downstream affects it has. This batch_connect_config merges with what folks supply in batch_connect options. I'd guess I want to be sure it has not unintended side-affects.

That said, I think we'd be OK, I just need to release something today urgently to fix a kinda critical bug.

[johrstrom]

I don't think we need default in the API.

@batch_connect_config.fetch(:ssh_allow, true) should be just fine. or even @batch_connect_config&.fetch(:ssh_allow, true) || true just to be extra double safe (though it looks like batch_connect_config should always be at least an empty {})

[treydock]

I fixed this, I think this was left over from an idea where the default would be pulled from the global env config, but I think chaining them all makes more sense.

[johrstrom]

OK I see where the merging happens here, so we're always merging batch_connect.basic or batch_connect.vnc. So other keys, like batch_connect.ssh_allow should be safe to add. I think just the update to the API and we're good to go.

ood_core/lib/ood_core/cluster.rb

Line 107 in 7aaad32

@batch_connect_config.fetch(template.to_sym, {}).to_h.symbolize_keys.merge(template: template.to_sym)

[johrstrom]

Thanks!