Skip to content
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

Adds auto_environment_variable smart attribute #3301

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/dashboard/Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ gem 'sdoc', group: :doc, require: false
group :development, :test do
# Call 'byebug' anywhere in the code to stop execution and get a debugger console
gem 'byebug'
gem 'pry'
gem 'climate_control', '~> 0.2'
gem 'timecop', '~> 0.9'
end
Expand Down
8 changes: 6 additions & 2 deletions apps/dashboard/app/controllers/scripts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def destroy
end
end

# POST /projects/:project_id/scripts/:id/save
# POST /projects/:project_id/:id/save
# save the script after editing
def save
@script.update(save_script_params[:script])
Expand Down Expand Up @@ -90,7 +90,11 @@ def submit_script_params
end

def save_script_params
params.permit({ script: SAVE_SCRIPT_KEYS }, :project_id, :id)
params.permit({ script: SAVE_SCRIPT_KEYS + auto_environment_variable_params }, :project_id, :id)
end

def auto_environment_variable_params
params[:script].keys.select { |key| key.match("auto_environment_variable") }
end

def find_project
Expand Down
9 changes: 8 additions & 1 deletion apps/dashboard/app/helpers/scripts_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ def create_editable_widget(form, attrib, format: nil)
locals = { form: form, attrib: attrib, format: format, fixed: fixed_attribute }

case widget
when 'number_field'
when 'number_field'
render(partial: editable_partial('editable_number'), locals: locals)
when 'select'
render(partial: editable_partial('editable_select'), locals: locals)
when 'key_value_pair'
render(partial: editable_partial('editable_key_value_pair'), locals: locals)
else
render(partial: editable_partial('generic'), locals: locals)
end
Expand Down Expand Up @@ -53,6 +55,11 @@ def auto_accounts_template
create_editable_widget(script_form_double, attrib)
end

def auto_environment_variable_template
attrib = SmartAttributes::AttributeFactory.build_auto_environment_variable
create_editable_widget(script_form_double, attrib)
end

# We need a form builder to build the template divs. These are
# templates so that they are not a part of the _actual_ form (yet).
# Otherwise you'd have required fields that you cannot actually edit
Expand Down
20 changes: 19 additions & 1 deletion apps/dashboard/app/javascript/script_edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ const newFieldData = {
bc_num_slots: {
label: "Nodes",
help: "How many nodes the job will run on."
},
auto_environment_variable: {
label: "Environment Variable",
help: "Add an environment variable."
}
}

Expand Down Expand Up @@ -65,7 +69,8 @@ function updateNewFieldOptions(selectMenu) {
const field = document.getElementById(`script_${newField}`);

// if the field doesn't already exist, it's an option for a new field.
if(field === null) {
// TODO: maybe JS equiv of ALLOW_MULTIPLE_FIELDS.include?(newField)
if(field === null || newField == "auto_environment_variable") {
const option = document.createElement("option");
option.value = newField;
option.text = newFieldData[newField].label;
Expand Down Expand Up @@ -143,11 +148,24 @@ function addInProgressField(event) {
justAdded.find('[data-fixed-toggler]')
.on('click', (event) => { toggleFixedField(event) });

justAdded.find('[data-multiple-input="name"]')
.on('change', (event) => { updateMultiple(event) });

const entireDiv = event.target.parentElement.parentElement.parentElement;
entireDiv.remove();
enableNewFieldButton();
}

function updateMultiple(event) {
const group = event.target.parentElement.parentElement;
event.target.id += `_${event.target.value}`
event.target.name = `script[${event.target.id.replace("script_", "")}]`;

let valueInput = group.children[1].children[1];
valueInput.id = event.target.id.replace("_name", "") + "_value";
valueInput.name = `script[${valueInput.id.replace("script_", "")}]`;
}

function fixExcludeBasedOnSelect(selectElement) {
const excludeElementId = selectElement.dataset.excludeId;
const selectOptions = Array.from(selectElement.options);
Expand Down
1 change: 1 addition & 0 deletions apps/dashboard/app/lib/smart_attributes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ module SmartAttributes
require 'smart_attributes/attributes/bc_queue'
require 'smart_attributes/attributes/bc_vnc_idle'
require 'smart_attributes/attributes/bc_vnc_resolution'
require 'smart_attributes/attributes/auto_environment_variable'
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
module SmartAttributes
class AttributeFactory
extend AccountCache

def self.build_auto_environment_variable(opts = {})
Attributes::AutoEnvironmentVariable.new('auto_environment_variable', opts)
end
end

module Attributes
class AutoEnvironmentVariable < Attribute
def widget
'key_value_pair'
end

def label(*)
(opts[:label] || 'Environment Variable').to_s
end

def options_to_serialize(fmt: nil)
opts[:job_environment]
end
end
end
end
58 changes: 48 additions & 10 deletions apps/dashboard/app/models/script.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,30 +61,44 @@ def initialize(opts = {})
@created_at = opts[:created_at]
sm_opts = {
form: opts[:form] || [],
attributes: opts[:attributes] || {}
attributes: opts[:attributes] || {},
job_environment: opts[:job_environment] || {},
}

add_required_fields(**sm_opts)

@smart_attributes = build_smart_attributes(**sm_opts)
end

def build_smart_attributes(form: [], attributes: {})
form.map do |form_item_id|
def build_smart_attributes(form: [], attributes: {}, job_environment: {})
attrs = form.map do |form_item_id|
Copy link
Contributor

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 the addition of job_environment here - it should work just like all the other smart attributes.

I think the structure of this is a bit off - like you're doing a lot of the work in this model when a lot of this parsing & updating logic should be embedded in the smart attribute itself.

I.e., it knows how to build itself and apply attributes to it.

So given a YAML like this:

form:
  - auto_environment_variable_FOO
attributes:
  auto_environment_variable_FOO:
    value: bar

That would call the factory method like

#  SmartAttributes::AttributeFactory.build(form_item_id, attrs) evaluates to 
SmartAttributes::AttributeFactory.build('auto_environment_variable_FOO', { value: 'bar' })

Then in the factory - we can do something similar to what auto_modules does - the factory parses out FOO and supplies it as an argument to the builder

if id.match?(AUTO_MODULES_REX)
hpc_mod = id.match(AUTO_MODULES_REX)[1]
id = 'auto_modules'
opts = opts.merge({'module' => hpc_mod})
end

attrs = attributes[form_item_id.to_sym].to_h.symbolize_keys
cache_value = cached_values[form_item_id]
attrs[:value] = cache_value if cache_value.present?
SmartAttributes::AttributeFactory.build(form_item_id, attrs)
end

unless job_environment.blank?
attrs << SmartAttributes::AttributeFactory.build('auto_environment_variable', job_environment)
end

attrs
end

def to_yaml
attributes = smart_attributes.each_with_object({}) do |sm, hash|
attributes = smart_attributes.reject do |attr|
attr.id == "auto_environment_variable"
end.each_with_object({}) do |sm, hash|
hash[sm.id.to_s] = sm.options_to_serialize
end.deep_stringify_keys

job_environment = smart_attributes.select { |attr| attr.id == "auto_environment_variable" }.first.try(:opts) || {}

hsh = { 'title' => title, 'created_at' => created_at }
hsh.merge!({ 'form' => smart_attributes.map { |sm| sm.id.to_s } })
hsh.merge!({ 'form' => smart_attributes.map { |sm| sm.id.to_s unless sm.id.to_s == "auto_environment_variable" }.compact })
hsh.merge!({ 'attributes' => attributes })
hsh.merge!({ 'job_environment' => job_environment.deep_stringify_keys }) unless job_environment.empty?

hsh.to_yaml
end

Expand Down Expand Up @@ -116,8 +130,12 @@ def respond_to_missing?(method_name, include_private = false)
end

def original_parameter(string)
match = /([\w_]+)_(?:min|max|exclude|fixed)/.match(string)
match[1]
if string.match('auto_environment_variable')
'auto_environment_variable'
else
match = /([\w_]+)_(?:min|max|exclude|fixed)/.match(string)
match[1]
end
end

# Find attribute in list using the id of the attribute
Expand Down Expand Up @@ -157,13 +175,14 @@ def destroy
end

def update(params)
# reset smart attributes becuase the user could have removed some fields
# reset smart attributes because the user could have removed some fields
@smart_attributes = []

# deal with things that would be in the 'form' section first to initialize
# the individual smart attributes
update_form(params)
update_attributes(params)
update_job_environment(params)
end

def submit(options)
Expand Down Expand Up @@ -206,7 +225,8 @@ def self.script_form_file(script_path)
# parameters you got from the controller that affect the attributes, not form.
# i.e., mins & maxes you set in the form but get serialized to the 'attributes' section.
def attribute_parameter?(name)
['min', 'max', 'exclude', 'fixed'].any? { |postfix| name && name.end_with?("_#{postfix}") }
['min', 'max', 'exclude', 'fixed'].any? { |postfix| name && name.end_with?("_#{postfix}") } ||
(name && name.match?('auto_environment_variable'))
end

# update the 'form' portion of the yaml file given 'params' from the controller.
Expand Down Expand Up @@ -236,6 +256,24 @@ def update_attributes(params)
end
end

def update_job_environment(params)
env_var_names = params.select { |k, _| k.match?('auto_environment_variable_name_') }
return if env_var_names.empty?

env_var_values = params.select do |param|
param.match?(/\Aauto_environment_variable_(.*)_value\Z/)
end

job_environment_hash = {}

env_var_values.each do |key, value|
variable_name = key.match(/\Aauto_environment_variable_(.*)_value\Z/)[1]
job_environment_hash[variable_name.to_s] = value.to_s
end

self[:auto_environment_variable] = SmartAttributes::AttributeFactory.build('auto_environment_variable', job_environment_hash)
end

def default_attributes(smart_attr_id)
case smart_attr_id
when 'auto_scripts'
Expand Down Expand Up @@ -308,7 +346,7 @@ def adapter(cluster_id)
OodAppkit.clusters[cluster_id] || raise(ClusterNotFound, "Job specifies nonexistent '#{cluster_id}' cluster id.")
end

def add_required_fields(form: [], attributes: {})
def add_required_fields(form: [], attributes: {}, job_environment: {})
add_cluster_to_form(form: form, attributes: attributes)
add_script_to_form(form: form, attributes: attributes)
end
Expand Down
4 changes: 4 additions & 0 deletions apps/dashboard/app/views/scripts/edit.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,7 @@
<template id="auto_accounts_template">
<%= auto_accounts_template %>
</template>

<template id="auto_environment_variable_template">
<%= auto_environment_variable_template %>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<%-
field_id = "#{form.object_name}_#{attrib.id}"
-%>

<div class="editable-form-field">
<div data-multiple-input="group">
<%= form.text_field :auto_environment_variable_name, data: { multiple_input: 'name' } %>
<%= form.text_field :auto_environment_variable_value, data: { multiple_input: 'value' } %>
</div>

<div class="d-none edit-group">
</div>

<%= render(partial: 'scripts/editable_form_fields/edit_field_buttons', locals: { field_id: field_id }) %>
</div>
40 changes: 38 additions & 2 deletions apps/dashboard/test/system/jobs_app_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ def add_bc_num_hours(project_id, script_id)
new_field_id = 'add_new_field_select'

actual_new_options = page.all("##{new_field_id} option").map(&:value).to_set
expected_new_options = ['bc_num_hours', 'auto_queues', 'bc_num_slots', 'auto_accounts'].to_set
expected_new_options = ['bc_num_hours', 'auto_queues', 'bc_num_slots', 'auto_accounts', 'auto_environment_variable'].to_set
assert_equal expected_new_options, actual_new_options
end
end
Expand Down Expand Up @@ -415,6 +415,30 @@ def add_bc_num_hours(project_id, script_id)
find('#script_bc_num_hours_fixed').click
find('#save_script_bc_num_hours').click

# add auto_environment_variable
click_on('Add new option')
select('Environment Variable', from: 'add_new_field_select')
click_on(I18n.t('dashboard.add'))
assert find('#script_auto_environment_variable_name')

fill_in('script_auto_environment_variable_name', with: 'SOME_VARIABLE')
fill_in('script_auto_environment_variable_value', with: 'some_value')

assert find('#script_auto_environment_variable_name_SOME_VARIABLE')
assert find('#script_auto_environment_variable_SOME_VARIABLE_value')

# add multiple auto_environment_variables
click_on('Add new option')
select('Environment Variable', from: 'add_new_field_select')
click_on(I18n.t('dashboard.add'))
assert find('#script_auto_environment_variable_name')

fill_in('script_auto_environment_variable_name', with: 'ANOTHER_VARIABLE')
fill_in('script_auto_environment_variable_value', with: 'some_other_value')

assert find('#script_auto_environment_variable_name_ANOTHER_VARIABLE')
assert find('#script_auto_environment_variable_ANOTHER_VARIABLE_value')

# correctly saves
click_on(I18n.t('dashboard.save'))
success_message = I18n.t('dashboard.jobs_scripts_updated')
Expand Down Expand Up @@ -459,9 +483,21 @@ def add_bc_num_hours(project_id, script_id)
label: Number of hours
help: ''
required: true
job_environment:
SOME_VARIABLE: some_value
ANOTHER_VARIABLE: some_other_value
HEREDOC

file = File.read("#{dir}/projects/#{project_id}/.ondemand/scripts/#{script_id}/form.yml")

assert_equal(expected_yml, File.read("#{dir}/projects/#{project_id}/.ondemand/scripts/#{script_id}/form.yml"))
assert_equal(expected_yml, file)

# correctly rebuilds form
# find("[href='#{edit_script_path}']").click

# shows all previously input fields
# assert find('#script_auto_environment_variable_name_SOME_VARIABLE')
# assert find('#script_auto_environment_variable_SOME_VARIABLE_value')
end
end

Expand Down