[PATCH] BZ#644283 Added validation check on format of pool name
by Martyn Taylor
From: Martyn Taylor <mtaylor(a)redhat.com>
---
src/app/controllers/pools_controller.rb | 10 ++++++----
src/app/models/pool.rb | 2 ++
src/app/views/pools/new.haml | 6 +++---
src/features/pool.feature | 7 +++++++
4 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/src/app/controllers/pools_controller.rb b/src/app/controllers/pools_controller.rb
index 1fdc047..2ba6940 100644
--- a/src/app/controllers/pools_controller.rb
+++ b/src/app/controllers/pools_controller.rb
@@ -103,10 +103,12 @@ class PoolsController < ApplicationController
@pool.quota_id = quota.id
@pool.zone = Zone.default
- @pool.save!
-
- flash[:notice] = "Pool added."
- redirect_to :action => 'show', :id => @pool.id
+ if @pool.save
+ flash[:notice] = "Pool added."
+ redirect_to :action => 'show', :id => @pool.id
+ else
+ render :action => :new
+ end
end
def manage_pool
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index cb03ddf..597128b 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -34,6 +34,8 @@ class Pool < ActiveRecord::Base
validates_presence_of :zone
validates_uniqueness_of :exported_as, :if => :exported_as
+ validates_format_of :name, :with => /^[\w -]*$/n, :message => "must only contain: numbers, letters, spaces, '_' and '-'"
+
has_many :permissions, :as => :permission_object, :dependent => :destroy,
:include => [:role],
:order => "permissions.id ASC"
diff --git a/src/app/views/pools/new.haml b/src/app/views/pools/new.haml
index 3b67b31..037c7c4 100644
--- a/src/app/views/pools/new.haml
+++ b/src/app/views/pools/new.haml
@@ -1,12 +1,12 @@
.grid_16
- = error_messages_for 'pool'
= error_messages_for 'account'
%h1 Create a new Pool
- - form_tag :action => 'create' do
+ - form_for @pool, :action => 'create' do |form|
%h2 Pool
%fieldset
%label.grid_2.alpha Name:
- = text_field :pool, :name, :class => "grid_5"
+ = form.text_field :name, :class => "grid_5"
+ = form.error_message_on :name, 'Name '
.clear.grid_14.prefix_2.alpha Provide a descriptive name for this pool.
= submit_tag "Save", :class => "submit formbutton"
diff --git a/src/features/pool.feature b/src/features/pool.feature
index 15abe09..b85b930 100644
--- a/src/features/pool.feature
+++ b/src/features/pool.feature
@@ -62,3 +62,10 @@ Feature: Manage Pools
Then I should see the following:
| Running Instances | 10 | 8 |
| Total Instances | 15 | 15 |
+
+ Scenario: Enter invalid characters into Name field
+ Given I am an authorised user
+ And I am on the new pool page
+ When I fill in "pool[name]" with "@%&*())_@!#!"
+ And I press "Save"
+ Then I should see "Name must only contain: numbers, letters, spaces, '_' and '-'"
\ No newline at end of file
--
1.7.2.3
13 years, 7 months
BZ#649389 Updated Login failed messaging on login page
by Martyn Taylor
This patch fixes the login message bug,
There is potential security issue with login messages highlighted here in BZ#652719
However, I am not sure this is high on our priority atm so for consistency I've used the standard way of displaying error messages
13 years, 7 months
Advocate for more and better testing
by lmartinc@redhat.com
Hi guys,
coming from strong test-driven development background I'm bit sad that in our Cloud Engine test we don't do testing as much as we could. Now we could polemize if the testing is so important so let's do that. I'm strong believer (with common-sense) and proponent of test driven development, but that's not what I want for you and for the project.
The fact that I prefer TDD with RSpec/BDD style of testing is my choice and I don't mind if somebody does it different way, but hey they at least write good tests. Having to write tests before the code is just matter of choice but writing test for the code is *must*.
Why *must*? :-)
More code, more people we add to the project more we have to explain, teach, "control" what gets in. With all that it will take more and more time to do our job. We as a team want to keep the velocity of our work as high as possible. And testing our codebase is one of the great ways to do that. I'm fully aware of that just testing ins't enough. But looking at the code I know we don't do any good job. A lot of us are not writing tests at all. Most of us not checking the tests they broke. Most of us don't care about green build at all. Too bad.
I would like you to encourage you to get better and more disciplined writing better code by writing it altogether with tests.
Tests will help you to:
1) make sure whatever you write is correct
2) document the code for others
3) do refactoring faster and also help you make sure u didn't broke anything what was working before
4) keep all your coworkers happy that you didn't broke their code
5) be better programmer ;)
To emphasize the need for tested code let me say that "It's not only about you anymore but about us.". Let's make life of the team and each other easier.
Feedback, critique, comments, opinions very appreciated.
Next week I want to write more about the way we test and try to get us all on the same level of testing if possible.
-- Ladislav
13 years, 7 months
[PATCH aggregator] add logger param to condormatic_classads_sync for use when invoked by condor_refreshd
by Mo Morsi
currently the messages output by condormatic in the context of condor_refreshd
goto the rails log.
---
src/app/util/condormatic.rb | 16 ++++++++--------
src/condor_refreshd/condor_refreshd | 2 +-
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
index 37040cd..263bb2c 100644
--- a/src/app/util/condormatic.rb
+++ b/src/app/util/condormatic.rb
@@ -157,8 +157,8 @@ def condormatic_instance_destroy(task)
end
-def condormatic_classads_sync
- Rails.logger.info "Starting condormatic_classads_sync..."
+def condormatic_classads_sync(logger=Rails.logger)
+ logger.info "Starting condormatic_classads_sync..."
index = 0
providers = Provider.find(:all)
@@ -182,7 +182,7 @@ def condormatic_classads_sync
# through the provider_combination_* ones. This is a bit
# racy, though, so I'm more inclined to just invalidate
# everything at present.
- Rails.logger.info "Starting classad invalidate..."
+ logger.info "Starting classad invalidate..."
begin
pipe = IO.popen("condor_advertise INVALIDATE_STARTD_ADS 2>&1", "w+")
pipe.puts 'MyType="Query"'
@@ -191,10 +191,10 @@ def condormatic_classads_sync
out = pipe.read
pipe.close
- Rails.logger.info "Did invalidate, output is #{out}"
+ logger.info "Did invalidate, output is #{out}"
if $? != 0
- Rails.logger.error "Unable to invalidate classads: #{out}"
+ logger.error "Unable to invalidate classads: #{out}"
raise "Unable to invalidate classads, classad sync failed"
end
rescue Errno::EPIPE
@@ -203,7 +203,7 @@ def condormatic_classads_sync
# classads, so we just go on
end
- Rails.logger.info "Syncing classads.."
+ logger.info "Syncing classads.."
ads = []
providers.each { |provider|
# The replicated image entry gets put in the database as soon as we ask
@@ -248,11 +248,11 @@ def condormatic_classads_sync
out = pipe.read
pipe.close
- Rails.logger.error "Unable to submit condor classad: #{out}" if $? != 0
+ logger.error "Unable to submit condor classad: #{out}" if $? != 0
index += 1
}
- Rails.logger.info "done"
+ logger.info "done"
end
def kick_condor
diff --git a/src/condor_refreshd/condor_refreshd b/src/condor_refreshd/condor_refreshd
index 4f1b93c..7b44fff 100755
--- a/src/condor_refreshd/condor_refreshd
+++ b/src/condor_refreshd/condor_refreshd
@@ -98,7 +98,7 @@ begin
while true
begin
- condormatic_classads_sync
+ condormatic_classads_sync(logger)
results = select([socket], nil, nil, timeout * 60)
# if results was nil, we timed out waiting, and we need to do a sync
--
1.7.2.3
13 years, 7 months
[PATCH aggregator 1/2] BZ #649843 - builds: column sorting not working
by Jan Provazník
From: Jan Provaznik <jprovazn(a)redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=649843
---
src/app/controllers/templates_controller.rb | 17 +++++++++++------
src/app/views/templates/builds.haml | 8 ++++----
2 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/src/app/controllers/templates_controller.rb b/src/app/controllers/templates_controller.rb
index 8261eab..55e41c9 100644
--- a/src/app/controllers/templates_controller.rb
+++ b/src/app/controllers/templates_controller.rb
@@ -11,12 +11,10 @@ class TemplatesController < ApplicationController
def index
# TODO: add template permission check
require_privilege(Privilege::IMAGE_VIEW)
- @order_dir = params[:order_dir] == 'desc' ? 'desc' : 'asc'
- @order_field = params[:order_field] || 'name'
@templates = Template.find(
:all,
:include => :images,
- :order => @order_field + ' ' + @order_dir
+ :order => get_order('name')
)
end
@@ -222,9 +220,10 @@ add account on <a href=\"#{url_for :controller => 'provider', \
end
def builds
- @running_images = Image.all(:include => :template, :conditions => ['status IN (?)', Image::ACTIVE_STATES])
- @completed_images = Image.all(:include => :template, :conditions => {:status => Image::STATE_COMPLETE})
- @failed_images = Image.all(:include => :template, :conditions => {:status => Image::STATE_FAILED})
+ order = get_order('templates.name')
+ @running_images = Image.all(:include => :template, :conditions => ['status IN (?)', Image::ACTIVE_STATES], :order => order)
+ @completed_images = Image.all(:include => :template, :conditions => {:status => Image::STATE_COMPLETE}, :order => order)
+ @failed_images = Image.all(:include => :template, :conditions => {:status => Image::STATE_FAILED}, :order => order)
require_privilege(Privilege::IMAGE_VIEW)
end
@@ -260,6 +259,12 @@ add account on <a href=\"#{url_for :controller => 'provider', \
private
+ def get_order(default)
+ @order_dir = params[:order_dir] == 'desc' ? 'desc' : 'asc'
+ @order_field = params[:order_field] || default
+ "#{@order_field} #{@order_dir}"
+ end
+
def flash_error(msg)
flash.now[:error] ||= {}
flash.now[:error][:summary] = 'Error while trying to build image'
diff --git a/src/app/views/templates/builds.haml b/src/app/views/templates/builds.haml
index e265634..0d2d5ce 100644
--- a/src/app/views/templates/builds.haml
+++ b/src/app/views/templates/builds.haml
@@ -1,8 +1,8 @@
- columns = [ |
- {:name => 'NAME', :sort_attr => 'template.name'}, |
- {:name => 'OS', :sort_attr => 'template.platform'}, |
- {:name => 'VERSION', :sort_attr => 'template.platform_version'}, |
- {:name => 'ARCH', :sort_attr => 'template.architecture'}, |
+ {:name => 'NAME', :sort_attr => 'templates.name'}, |
+ {:name => 'OS', :sort_attr => 'templates.platform'}, |
+ {:name => 'VERSION', :sort_attr => 'templates.platform_version'}, |
+ {:name => 'ARCH', :sort_attr => 'templates.architecture'}, |
{:name => 'STATUS', :sort_attr => 'status'}, |
] |
--
1.7.2.3
13 years, 7 months
[PATCH] BZ#644283 Added validation check on format of pool name
by Martyn Taylor
From: Martyn Taylor <mtaylor(a)redhat.com>
---
src/app/controllers/pools_controller.rb | 10 ++++++----
src/app/models/pool.rb | 2 ++
src/app/views/pools/new.haml | 6 +++---
src/features/pool.feature | 7 +++++++
4 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/src/app/controllers/pools_controller.rb b/src/app/controllers/pools_controller.rb
index 1fdc047..2ba6940 100644
--- a/src/app/controllers/pools_controller.rb
+++ b/src/app/controllers/pools_controller.rb
@@ -103,10 +103,12 @@ class PoolsController < ApplicationController
@pool.quota_id = quota.id
@pool.zone = Zone.default
- @pool.save!
-
- flash[:notice] = "Pool added."
- redirect_to :action => 'show', :id => @pool.id
+ if @pool.save
+ flash[:notice] = "Pool added."
+ redirect_to :action => 'show', :id => @pool.id
+ else
+ render :action => :new
+ end
end
def manage_pool
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index cb03ddf..597128b 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -34,6 +34,8 @@ class Pool < ActiveRecord::Base
validates_presence_of :zone
validates_uniqueness_of :exported_as, :if => :exported_as
+ validates_format_of :name, :with => /^[\w -]*$/n, :message => "must only contain: numbers, letters, spaces, '_' and '-'"
+
has_many :permissions, :as => :permission_object, :dependent => :destroy,
:include => [:role],
:order => "permissions.id ASC"
diff --git a/src/app/views/pools/new.haml b/src/app/views/pools/new.haml
index 3b67b31..037c7c4 100644
--- a/src/app/views/pools/new.haml
+++ b/src/app/views/pools/new.haml
@@ -1,12 +1,12 @@
.grid_16
- = error_messages_for 'pool'
= error_messages_for 'account'
%h1 Create a new Pool
- - form_tag :action => 'create' do
+ - form_for @pool, :action => 'create' do |form|
%h2 Pool
%fieldset
%label.grid_2.alpha Name:
- = text_field :pool, :name, :class => "grid_5"
+ = form.text_field :name, :class => "grid_5"
+ = form.error_message_on :name, 'Name '
.clear.grid_14.prefix_2.alpha Provide a descriptive name for this pool.
= submit_tag "Save", :class => "submit formbutton"
diff --git a/src/features/pool.feature b/src/features/pool.feature
index 15abe09..b85b930 100644
--- a/src/features/pool.feature
+++ b/src/features/pool.feature
@@ -62,3 +62,10 @@ Feature: Manage Pools
Then I should see the following:
| Running Instances | 10 | 8 |
| Total Instances | 15 | 15 |
+
+ Scenario: Enter invalid characters into Name field
+ Given I am an authorised user
+ And I am on the new pool page
+ When I fill in "pool[name]" with "@%&*())_@!#!"
+ And I press "Save"
+ Then I should see "Name must only contain: numbers, letters, spaces, '_' and '-'"
\ No newline at end of file
--
1.7.2.3
13 years, 7 months
[PATCH aggregator] BZ #649852 - clicking instance details without one selected error message
by Jan Provazník
From: Jan Provaznik <jprovazn(a)redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=649852
---
src/app/controllers/instance_controller.rb | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/instance_controller.rb b/src/app/controllers/instance_controller.rb
index 0de246d..6b19f26 100644
--- a/src/app/controllers/instance_controller.rb
+++ b/src/app/controllers/instance_controller.rb
@@ -136,7 +136,13 @@ class InstanceController < ApplicationController
end
end
- @instance = Instance.find((params[:id] || []).first)
+ if params[:id].blank?
+ flash[:warning] = "No instance selected"
+ redirect_to :action => 'index'
+ return
+ end
+
+ @instance = Instance.find(params[:id].to_a.first)
require_privilege(Privilege::INSTANCE_CONTROL,@instance.pool)
if params[:instance_details]
--
1.7.2.3
13 years, 7 months