That's a good point, but I've ACK'd and pushed anyway, since it's a blocker and the issue mentioned isn't new.
Mainn ----- Original Message ----- > From: "Jason Guiditta" <[email protected]> > To: [email protected] > Cc: [email protected] > Sent: Monday, October 22, 2012 11:21:33 AM > Subject: Re: [PATCH conductor] BZ 865833 - Prevent editting vsphere provider > name > > On 18/10/12 17:43 +0200, [email protected] wrote: > >From: Jozef Zigmund <[email protected]> > > > >Used the same way as for rhevm provider name > > > >https://bugzilla.redhat.com/show_bug.cgi?id=865833 > >--- > > src/app/models/provider.rb | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > >diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb > >index fb2ac84..24828e1 100644 > >--- a/src/app/models/provider.rb > >+++ b/src/app/models/provider.rb > >@@ -74,7 +74,9 @@ class Provider < ActiveRecord::Base > > false > > end > > when "RHEV-M" > >- load_rhevm_json(name) > >+ load_json(name, "rhevm") > >+ when "VMware vSphere" > >+ load_json(name, "vsphere") > > when "Amazon EC2" > > if name.starts_with?("ec2-") > > true > >@@ -300,8 +302,8 @@ class Provider < ActiveRecord::Base > > true > > end > > > >- def load_rhevm_json(provider_name) > >- path_to_json = "/etc/imagefactory/rhevm.json" > >+ def load_json(provider_name, provider_type) > >+ path_to_json = "/etc/imagefactory/#{provider_type}.json" > > if File.exists?(path_to_json) > > json = File.read(path_to_json) > > json_hash = ActiveSupport::JSON.decode(json) > >-- > >1.7.11.7 > > > Please don't put hard-coded paths into our models like this. I won't > get into the larger issue of conditionally doing things based on > provider (or any, really) string, as this patch does not introduce > that particular issue, but we should get this path in some other way. > For while these things (system-setting factory config files) exist, > perhaps something like a setting in the initializer (I would guess > this is settings.yml, but whatever one applies for factory) would be > a > simple compromise for the time being. Then we can have a default > that > is easy to override, either manually or via configure. > > Marginally related, it seems like there might be a security hole for > conductor can read files in a director owned by factory, but perhaps > I > am concerned about nothing there..? > > -j >
