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.

Sign up for GitHub

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

Jump to bottom

Added template files sync when creating a project from a template #3031

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Added template files sync when creating a project from a template #3031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/app/controllers/projects_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def create
else
message = @project.errors[:save].empty? ? I18n.t('dashboard.jobs_project_validation_error') : I18n.t('dashboard.jobs_project_generic_error', error: @project.collect_errors)
flash.now[:alert] = message
@templates = templates if project_params.key?(:template)
render :new
end
end
Expand Down
43 changes: 33 additions & 10 deletions apps/dashboard/app/models/project.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ def templates

template_dir.children.map do |child_directory|
opts = {
# Fake id is needed to make it a valid project
id: '__template',
directory: child_directory
}

Expand All @@ -63,7 +65,7 @@ def templates
end
end

attr_reader :directory, :id
attr_reader :directory, :id, :template

delegate :icon, :name, :description, to: :manifest

Expand All @@ -72,6 +74,7 @@ def templates
validates :icon, format: { with: %r{\Afa[bsrl]://[\w-]+\z}, allow_blank: true, message: :format }, on: [:create, :update]
validate :project_directory_invalid, on: [:create, :update]
validate :project_directory_exist, on: [:create]
validate :project_template_invalid, on: [:create]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't necessarily specific to this PR - but why are we using :create here? There's no script#create API.

How does this work when nobody is calling or defined a create API?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see, we call valid?(:create) somewhere. We can defer this, but that seems confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update it to :save to be consistent with the model method call when validating

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it in another PR - I think I'm good with this.


# the template you created this project from
attr_accessor :template
Expand All @@ -80,6 +83,7 @@ def initialize(attributes = {})
@id = attributes.delete(:id)
@directory = attributes.delete(:directory)
@directory = File.expand_path(@directory) unless @directory.blank?
@template = attributes.delete(:template)

@manifest = Manifest.new(attributes)
@manifest = @manifest.merge(Manifest.load(manifest_path)) unless new_record?
Expand All @@ -105,23 +109,22 @@ def save(attributes={})
@directory = Project.dataroot.join(id.to_s).to_s if directory.blank?
@manifest = manifest.merge({ icon: 'fas://cog' }) if icon.blank?

make_dir
store_manifest
make_dir && sync_template && store_manifest(:save)
end

def update(attributes)
@manifest = manifest.merge(attributes)

return false unless valid?(:update)

store_manifest
store_manifest(:update)
end

def store_manifest
def store_manifest(operation)
if manifest.valid? && manifest.save(manifest_path) && add_to_lookup
true
else
errors.add(:save, I18n.t('dashboard.jobs_project_save_error', path: manifest_path))
errors.add(operation, I18n.t('dashboard.jobs_project_save_error', path: manifest_path))
false
end
end
Expand Down Expand Up @@ -171,9 +174,7 @@ def manifest_path
end

def collect_errors
errors.map do |field_error|
field_error.message()
end.join(', ')
errors.map(&:message).join(', ')
end

private
Expand All @@ -183,8 +184,23 @@ def collect_errors
def make_dir
project_dataroot.mkpath unless project_dataroot.exist?
configuration_directory.mkpath unless configuration_directory.exist?
true
rescue StandardError => e
errors.add(:save, "Failed to make directory: #{e.message}")
false
end

def sync_template
return true if template.blank?

# Sync the template files over
oe, s = Open3.capture2e("rsync", "-a", "#{template}/", "#{project_dataroot}")
raise oe unless s.success?

true
rescue StandardError => e
errors.add(:make_directory, "Failed to make directory: #{e.message}")
errors.add(:save, "Failed to sync template: #{e.message}")
false
end

def project_directory_exist
Expand All @@ -198,4 +214,11 @@ def project_directory_invalid
errors.add(:directory, :invalid)
end
end

def project_template_invalid
# This validation is to prevent the template directory being manipulated in the form.
if !template.blank? && Project.templates.map { |template| template.directory.to_s }.exclude?(template.to_s)
errors.add(:template, :invalid)
end
end
end
66 changes: 54 additions & 12 deletions apps/dashboard/test/models/projects_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,17 @@
require 'test_helper'

class ProjectsTest < ActiveSupport::TestCase
test 'crate project validation' do
test 'create empty project' do
project = Project.new

assert_nil project.id
assert_nil project.directory
assert_equal '', project.name
assert_equal '', project.description
assert_equal '', project.icon
end

test 'create project validation' do
Dir.mktmpdir do |tmp|
OodAppkit.stubs(:dataroot).returns(Pathname.new(tmp))

Expand All @@ -13,12 +23,13 @@ class ProjectsTest < ActiveSupport::TestCase
assert_not project.errors[:name].empty?

invalid_directory = Project.dataroot
project = Project.new({ name: 'test', icon: 'invalid_format', directory: invalid_directory.to_s })
project = Project.new({ name: 'test', icon: 'invalid_format', directory: invalid_directory.to_s, template: '/invalid/template' })

assert_not project.save
assert_equal 2, project.errors.size
assert_equal 3, project.errors.size
assert_not project.errors[:icon].empty?
assert_not project.errors[:directory].empty?
assert_not project.errors[:template].empty?
end
end

Expand All @@ -32,6 +43,41 @@ class ProjectsTest < ActiveSupport::TestCase
end
end

test 'creates project with directory override' do
Dir.mktmpdir do |tmp|
projects_root = Pathname.new(tmp)
project_dir = File.join(tmp, 'dir_override')
project = create_project(projects_root, directory: project_dir)

assert_equal project_dir, project.directory
assert File.directory?(Pathname.new(project_dir))
assert File.file?(Pathname.new("#{project_dir}/.ondemand/manifest.yml"))
end
end

test 'creates project with template copies template files' do
Dir.mktmpdir do |tmp|
template_dir = File.join(tmp, 'template')
file_content = <<~HEREDOC
some multiline content
echo 'multiline content'
description: multiline content
HEREDOC
Pathname.new(template_dir).mkpath
File.open("#{template_dir}/script.sh", 'w') { |file| file.write(file_content) }
File.open("#{template_dir}/info.txt", 'w') { |file| file.write(file_content) }
File.open("#{template_dir}/config.yml", 'w') { |file| file.write(file_content) }

Configuration.stubs(:project_template_dir).returns(tmp)
projects_path = Pathname.new(tmp)
project = create_project(projects_path, template: template_dir)

assert Dir.entries(project.directory).include?('script.sh')
assert Dir.entries(project.directory).include?('info.txt')
assert Dir.entries(project.directory).include?('config.yml')
end
end

test 'creates .ondemand configuration directory' do
Dir.mktmpdir do |tmp|
projects_path = Pathname.new(tmp)
Expand Down Expand Up @@ -96,12 +142,8 @@ class ProjectsTest < ActiveSupport::TestCase
description: my test project
HEREDOC

puts "project: #{project.inspect}"

assert project.update(test_attributes)

puts "project: #{project.inspect}"

assert File.exist?(project.manifest_path.to_s)

assert_equal expected_manifest_yml, File.read(project.manifest_path.to_s)
Expand All @@ -115,13 +157,14 @@ class ProjectsTest < ActiveSupport::TestCase
old_id = project.id
old_directory = project.directory

assert project.update({ id: 'updated', name: 'updated', icon: 'fas://updated', directory: '/updated', description: 'updated' })
assert project.update({ id: 'updated', name: 'updated', icon: 'fas://updated', directory: '/updated', description: 'updated', template: '/some/path' })
assert_equal 'updated', project.name
assert_equal 'fas://updated', project.icon
assert_equal 'updated', project.description

assert_equal old_id, project.id
assert_equal old_directory, project.directory
assert_nil project.template
end
end

Expand All @@ -137,13 +180,12 @@ class ProjectsTest < ActiveSupport::TestCase
end
end

def create_project(projects_path, name: 'test-project', icon: 'fas://arrow-right', description: 'description', directory: nil)
def create_project(projects_path, name: 'test-project', icon: 'fas://arrow-right', description: 'description', directory: nil, template: nil)
OodAppkit.stubs(:dataroot).returns(projects_path)
id = Project.next_id
directory = Project.dataroot.join(id.to_s).to_s if directory.blank?
attrs = { name: name, icon: icon, id: id, description: description, directory: directory }
attrs = { name: name, icon: icon, id: id, description: description, directory: directory, template: template }
project = Project.new(attrs)
assert project.save
assert project.save, project.collect_errors

project
end
Expand Down