Created default executable script on new Script and added validation #3273
Conversation
|
Sorry for the delay on this, I'll be sure to look it over shortly. |
|
Sorry, we're really felling the pressure for 3.1 so I may not get to this quickly as it's not really required for 3.1. |
apps/dashboard/app/models/script.rb
Outdated
|
|
||
| script_content = <<~EOF | ||
| #!/bin/bash | ||
| # Sample script to configure project defaults. Delete when other scrips available. |
There was a problem hiding this comment.
Should be "Delete when other scripts are available."
| @valid_scripts = Script.scripts?(@project.directory) | ||
|
|
||
| alert_messages = [] | ||
| alert_messages << I18n.t('dashboard.jobs_project_invalid_configuration_clusters') unless @valid_project | ||
| alert_messages << I18n.t('dashboard.jobs_project_invalid_configuration_scripts') if @scripts.any? && !@valid_scripts | ||
| flash.now[:alert] = alert_messages.join(' ') if alert_messages.any? |
There was a problem hiding this comment.
I cannot seem to get these alerts to show up. But honestly, if it creates a hello_world.sh automatically - I wonder if we need them at all?
I think creating the default script without asking/or alerting the user is actually a good first step. Maybe we could build in the alert/prompt later - but I'm now wondering what value it provides?
Maybe actually - just a note on the success notice like -
There was a problem hiding this comment.
The script error message will appear if there are Scripts created and afterwards, the user manually deletes all the executable scripts from the project. Then, when rendering the projects page, the error message will show.
As scripts are added/removed by the user with the file upload tool, it is possible that the user deletes all executables.
There was a problem hiding this comment.
The script error message will appear if there are Scripts created and afterwards, the user manually deletes all the executable scripts from the project.
That was the step I was missing! Yes I'm now able to get the alert to flash.
OK - While I'm fine for this for now, I would still make a ticket to make this flash a modal that you says something to the same effect, where you can click to create one.
An executable shell script is required for your project.
Would you like to create a hello world shell script?
[yes] [no]
If you want to pick that up now or later, either is fine with me.
There was a problem hiding this comment.
I think later is better.
I am not sure about the modal as it will keep appearing and forcing the user to select an option every time the projects page is displayed (if they select no and until a script is created).
There was a problem hiding this comment.
OK.
I am not sure about the modal as it will keep appearing and forcing the user to select an option every time the projects page is displayed (if they select no and until a script is created).
That's fine by me. They need a script for any of this to work, so it's either our modal telling them that or the sbatch or similar failure message when they attempt to start a job - which they may or may not understand.
There was a problem hiding this comment.
Added the default script created message
apps/dashboard/app/models/script.rb
Outdated
| return if Script.scripts?(project_dir) | ||
| return if default_script_path.exist? |
There was a problem hiding this comment.
Can you combine these into return if Script.scripts?(project_dir) || default_script_path.exist?. Little bit of a nit-pick, but I just think that's the ruby way (whatever that means).
Creates a default executable script in the project's directory when a new Script is created.
As well, it adds a validation message when there are no executable scripts in the project.
Fixes #3255