Mailing-List: contact dev-help@deltacloud.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@deltacloud.apache.org
Received-SPF: pass (athena.apache.org: domain of lutter@redhat.com designates
 209.132.183.28 as permitted sender)
Message-ID: <1336687612.2686.59.camel@avon.watzmann.net>
Subject: Re: [PATCH core 1/2] EC2: Added initial support for EC2 frontend
From: David Lutterkort <lutter@redhat.com>
To: dev@deltacloud.apache.org
Date: Thu, 10 May 2012 15:06:52 -0700
In-Reply-To: <1336663704-58834-2-git-send-email-mfojtik@redhat.com>
References: <1336663704-58834-1-git-send-email-mfojtik@redhat.com>
	 <1336663704-58834-2-git-send-email-mfojtik@redhat.com>
Organization: Red Hat Inc
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0

On Thu, 2012-05-10 at 17:28 +0200, mfojtik@redhat.com wrote:

> diff --git a/server/lib/ec2/helpers.rb b/server/lib/ec2/helpers.rb
> new file mode 100644
> index 0000000..32380fa
> --- /dev/null
> +++ b/server/lib/ec2/helpers.rb
> @@ -0,0 +1,29 @@
> +
> +require_relative '../deltacloud/helpers/driver_helper'
> +require_relative '../deltacloud/helpers/auth_helper'
> +require_relative '../deltacloud/core_ext/string'
> +require_relative '../deltacloud/core_ext/array'
> +require_relative '../deltacloud/core_ext/hash'
> +require_relative '../deltacloud/core_ext/integer'
> +require_relative '../deltacloud/core_ext/proc'

I wonder if we shouldn't just load that all in config.ru (or have a
general file that every frontend requires)

> diff --git a/server/lib/ec2/helpers/convertor.rb b/server/lib/ec2/helpers/convertor.rb
> new file mode 100644
> index 0000000..9d98e5b
> --- /dev/null
> +++ b/server/lib/ec2/helpers/convertor.rb
> @@ -0,0 +1,140 @@
> +# Licensed to the Apache Software Foundation (ASF) under one or more
> +# contributor license agreements.  See the NOTICE file distributed with
> +# this work for additional information regarding copyright ownership.  The
> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance with the
> +# License.  You may obtain a copy of the License at
> +#
> +#       http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +
> +module Deltacloud::EC2
> +
> +  class Convertor
> +
> +    def self.convert(builder, action, result)
> +      klass_name = ActionHandler::MAPPINGS[action][:method].to_s.camelize
> +      klass = Convertor.const_get(klass_name)
> +      klass.new(builder, result).to_xml
> +    end

Are you sure this is simpler than explicit HAML templates to generate
XML ? Since we are storing the driver result in a model object anyway, a
template might be clearer. (Minor nit: it's converter rather than
convertor)

> diff --git a/server/lib/ec2/query_parser.rb b/server/lib/ec2/query_parser.rb
> new file mode 100644
> index 0000000..53c7d61
> --- /dev/null
> +++ b/server/lib/ec2/query_parser.rb
> @@ -0,0 +1,129 @@
> +# Licensed to the Apache Software Foundation (ASF) under one or more
> +# contributor license agreements.  See the NOTICE file distributed with
> +# this work for additional information regarding copyright ownership.  The
> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance with the
> +# License.  You may obtain a copy of the License at
> +#
> +#       http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +
> +module Deltacloud::EC2
> +
> +  class ActionHandler
> +
> +    MAPPINGS = {
> +      :describe_availability_zones => { :method => :realms, :params => { 'ZoneName.1' => :id } },
> +      :describe_images => { :method => :images, :params => { 'ImageId.1' => :id }},
> +      :describe_instances => { :method => :instances, :params => {} },
> +      :run_instances => { :method => :create_instance, :params => { 'ImageId' => :image_id, 'InstanceType' => :hwp_id, 'Placement.AvailabilityZone' => :realm_id }}
> +    }

This is screaming "invent a DSL for me" ;) But I'd hold off until we
have a PoC that's fully working across a few drivers.

> +    def initialize(request_id, params={})
> +      @request_id = request_id
> +      @action = (params.delete('Action') || '').underscore.intern

You can't intern an empty string - not having an Action should produce
an error anyway.

David