JayG,
Hi Jayg, no worries. feedback is greatly appreciated :)
So I agree with some of the minor points you mentioned like renaming
vars/config/methodname etc... But not so much with the code itself, I've left
comments inline. thanks :)
----- Original Message -----
From: "Jason Guiditta" <jguiditt(a)redhat.com>
To: mtaylor(a)redhat.com
Cc: aeolus-devel(a)fedorahosted.org
Sent: Monday, 13 June, 2011 10:30:16 PM
Subject: Re: [PATCH aeolus 1/2] Added RestClient Resources for imagefactory/conductor
On Mon, 2011-06-13 at 16:22 +0100, mtaylor(a)redhat.com wrote:
From: Martyn Taylor <mtaylor(a)redhat.com>
Man, I sure feel like a pain, but there are still a few issues I have
with this patch, inline.
---
.../image_factory/aeolus-image/config/cli.yaml | 7 +++
.../aeolus-image/config/imagefactory_cli | 8 +++
.../image_factory/aeolus-image/lib/base_command.rb | 56 ++++++++++++++++++++
.../aeolus-image/spec/base_command_spec.rb | 26 +++++++++
.../aeolus-image/spec/fixtures/imagefactory-cli | 7 +++
5 files changed, 104 insertions(+), 0 deletions(-)
create mode 100644 services/image_factory/aeolus-image/config/cli.yaml
create mode 100644 services/image_factory/aeolus-image/config/imagefactory_cli
create mode 100644 services/image_factory/aeolus-image/spec/base_command_spec.rb
create mode 100644 services/image_factory/aeolus-image/spec/fixtures/imagefactory-cli
diff --git a/services/image_factory/aeolus-image/config/cli.yaml
b/services/image_factory/aeolus-image/config/cli.yaml
new file mode 100644
index 0000000..b83862e
--- /dev/null
+++ b/services/image_factory/aeolus-image/config/cli.yaml
@@ -0,0 +1,7 @@
+imagefactory:
+ url:
http://localhost:9090
+
+conductor:
+ url:
http://localhost:3000/conductor
+ username: admin
+ password: password
\ No newline at end of file
diff --git a/services/image_factory/aeolus-image/config/imagefactory_cli
b/services/image_factory/aeolus-image/config/imagefactory_cli
new file mode 100644
index 0000000..9ab77bb
--- /dev/null
+++ b/services/image_factory/aeolus-image/config/imagefactory_cli
@@ -0,0 +1,8 @@
+# Place this file in: ~/.imagefactory-cli
+imagefactory:
Again, this ^ should be iwhd rather than imagefactory
OK I can change the name of this variable.
+ url:
http://localhost:9090
+
+conductor:
+ url:
http://localhost:3000/conductor
+ username: admin
+ password: password
Why do we need 2 copies of the above file? Also, the imagefactory_cli
version does not match the expected imagefactory-cli referenced in both
the source and the test, so this through me when copying it over to ~,
as I also had to rename it. If we are going to provider a sample file,
I think we should have it already be the right name (though I can see
not prepending it with '.'). I also think we should keep our sample
data/fixture type stuff in one folder. As we already have a
spec/sample_data folder, I would like to see this go there (though I am
open to giving it a better name if that makes more sense). Lastly for
this topic, let's change the name to aeolus-cli, as we discussed in irc.
One is for the tests (in fixtures), the other is an example config for the user to cp to
~/, I realized after I sent the patch that the tests did not override the initialize
method, as so used the config in fixtures.
Its the tests that need updating here. I can do that this morning.
diff --git a/services/image_factory/aeolus-image/lib/base_command.rb
b/services/image_factory/aeolus-image/lib/base_command.rb
index 4d4fa42..3bf1d4d 100644
--- a/services/image_factory/aeolus-image/lib/base_command.rb
+++ b/services/image_factory/aeolus-image/lib/base_command.rb
@@ -1,11 +1,19 @@
+require 'yaml'
+require 'rest_client'
+require 'nokogiri'
+
module Aeolus
module Image
#This will house some methods that multiple Command classes need to use.
class BaseCommand
+ @@config_location = ENV['HOME'] + "/.imagefactory-cli"
This should not be a class-level variable. In fact, I don't think we
need a variable at all, we should just add the file-checking to the
ConfigParser and have it look where the user specified with the --config
flag as well as in one default location (which would be just a slight
variant of the above).
IMO If this is a variable then it should be class-level. Since it only gets loaded once
when the class is loaded, instead of each time it's initialized. I suppose that
depends on how we are using the client. I guess if we're loading the libs each time
we send a request then it makes no difference. I will remove it altogether if you feel it
looks untidy.
+
attr_accessor :options
+
def initialize(opts={}, logger=nil)
logger(logger)
@options = opts
+ @config = load_config
Related comment to above, let's move this into
the config parser. It
should just dump whatever is in the file into the options hash, so it
will also do away with much of the conditional logic below.
There is no logic in load_config?
end
protected
@@ -19,6 +27,54 @@ module Aeolus
return @logger
end
For these 2 methods, the thing passed to create_resource should be the
symbol from the options hash
Really? These are keys in the config file? And these helper methods are intended for use
by the appropriate command, (some methods may need to perform actions on both
imagefactory(iwhd) and conductor.
+ def imagefactory
+ createResource('imagefactory')
+ end
+
+ def conductor
+ createResource('conductor')
+ end
+
+ private
+ def load_config
+ begin
+ YAML::load( File.open(@@config_location))
+ rescue Errno::ENOENT
+ #TODO: Create a custom exception to wrap CLI Exceptions
+ raise "Unable to locate configuration file: \"" + @@config_url
+ "\""
+ end
+ end
+
ruby standard for methods is nonCamelCase :) so this becomes
'create_resource'
Whoops that one slipped in, been doing some java recently ;)
+ def createResource(resource_name)
+ # Check to see if config has a resource with this name
+ if !(a)config.has_key?(resource_name)
I think this would be more readable
as:
unless config.has_key?(resource_name)
+ raise "Unable to determine resource: " +
resource_name + " from configuraion file. Please check: " + @@config_location
+ return
+ end
+
The following 2 chunks should be combined and we should always use the
options hash.
Combining these chunks changes the functionality of the method. We want to check to see
if a user supplies arguments via the command line. If a username is supplied, then we
make the decision at this point* to use authentication from the command line, if the user
then fails to supply a password then we throw an error.
Combining these chunks would mean that supplying a username, but not password via command
line would default to use the credentials supplied in the config file. The same behaviour
applies for when using the config.
+ #Use command line arguments for username/password
+ if @options.has_key?('username')
+ if @options.has_key?('password')
I would collapse the above
checks into one if statement look for un &&
pw. I don't see it being an error to not supply them, the resource we
are hitting will give us a 401 if they are needed, and we just need to
surface that error to the user and quit out.
If a user supplies a username then we require a password, if they dont supply a username
then it will default to the next level.
+
RestClient::Resource.new(@config[resource_name]['url'], :user =>
@options['username'], :password => @options['password'])
+ else
+ #TODO: Create a custom exception to wrap CLI Exceptions
+ raise "Password not found for user: " +
@options['username']
+ end
+
+ #Use config for username/password
+ elsif @config[resource_name].has_key?('username')
+ if @config[resource_name].has_key?('password')
+ RestClient::Resource.new(@config[resource_name]['url'], :user =>
@config[resource_name]['username'], :password =>
@config[resource_name]['password'])
+ else
+ #TODO: Create a custom exception to wrap CLI Exceptions
+ raise "Password not found for user: " +
@config[resource_name]['username']
+ end
+
+ # Do not use authentication
+ else
+ RestClient::Resource.new(@config[resource_name]['url'])
+ end
+ end
end
end
end
diff --git a/services/image_factory/aeolus-image/spec/base_command_spec.rb
b/services/image_factory/aeolus-image/spec/base_command_spec.rb
new file mode 100644
index 0000000..3c9cada
--- /dev/null
+++ b/services/image_factory/aeolus-image/spec/base_command_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+module Aeolus
+ module Image
+ describe ConfigParser do
+ before(:each) do
+ BaseCommand.stub!(:load_config).and_return(YAML::load(
File.open(File.join(File.dirname(__FILE__), "fixtures/imagefactory-cli"))))
I think the directory above should be updated to sample_data/aeolus-cli,
otherwise we end up with 2 copies of the same sample file.
Yup that's cool with me
+ end
+
+ it "should determine the correct credentials for HTTP Authentication"
do
+ basec = BaseCommand.new
+ imagefactory = basec.send :imagefactory
+ imagefactory.user.should == nil
+
+ conductor = basec.send :conductor
+ conductor.user.should == "admin"
+ conductor.password.should == "password"
+
+ basec = BaseCommand.new({'username' => "testusername",
'password'=> "testpassword"})
+ conductor = basec.send :conductor
+ conductor.user.should == "testusername"
+ conductor.password.should == "testpassword"
+ end
+ end
I would suggest a few changes here based on my above feedback. There
should be 2 tests added to the config parser spec to check for proper
finding and parsing of the file if one is passed in or if it exists in
the default location (passed in should override default).
OK this means I need to add the ability to pass in a config file via the command line if I
want to test that but the actual test, tests all those things you mentioned. You want it
splitting into two tests?
For the actual tests for baseCommand, we should not need to test un/pw,
as they are already set. If we want to stay away from hitting the
resource, we can just do away with these tests. OTOH, if we want to
leave the dependency between components for now (which I kind of think
is ok), we can test against default values in the file, assuming the
developer has everything set up on localhost. Then in an upcoming
revision, we can set up some fake data/responses to be returned by vcr
or similar library.
This method is actually testing three things:
1) That the config is parsed properly
2) That the control flow through the load_config method is correct
3) The rest-client is created properly
> + end
> +end
> \ No newline at end of file
> diff --git a/services/image_factory/aeolus-image/spec/fixtures/imagefactory-cli
b/services/image_factory/aeolus-image/spec/fixtures/imagefactory-cli
> new file mode 100644
> index 0000000..b83862e
> --- /dev/null
> +++ b/services/image_factory/aeolus-image/spec/fixtures/imagefactory-cli
> @@ -0,0 +1,7 @@
> +imagefactory:
+ url:
http://localhost:9090
+
+conductor:
+ url:
http://localhost:3000/conductor
+ username: admin
+ password: password
> \ No newline at end of file
Once more, apologies for so much feedback, but I could not explain my
thoughts in any other way.
Given our TZ differences, I think lots of feedback is a good thing.
Thanks
Martyn
-j