-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cluster batch_connect_ssh_allow? function to allow easier access to a new setting #286
Conversation
…to a new setting: batch_connect: ssh_allow: <boolean> Needed for OSC/ondemand#1157
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. |
Sorry, not per job, I meant per app. |
Could always chain things so that to show the link |
@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 everything defaults to |
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 That said, I think we'd be OK, I just need to release something today urgently to fix a kinda critical bug. |
lib/ood_core/cluster.rb
Outdated
return @batch_connect_ssh_allow = default if batch_connect_config.nil? | ||
|
||
@batch_connect_ssh_allow = batch_connect_config.fetch(:ssh_allow, default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
OK I see where the merging happens here, so we're always merging ood_core/lib/ood_core/cluster.rb Line 107 in 7aaad32
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Add cluster batch_connect_ssh_allow? function to allow easier access to a new setting:
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.