[PATCH conductor] bug 786220 - Scope images to Pool Family

Matt Wagner matt.wagner at redhat.com
Wed Feb 15 22:47:06 UTC 2012


On Wed, Feb 15, 2012 at 01:51:38PM -0500, Scott Seago wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=786220
> ---
>  src/app/controllers/api/environments_controller.rb |   39 ++++++++
>  src/app/controllers/api/images_controller.rb       |   58 ++++++++++--
>  .../controllers/api/provider_images_controller.rb  |    4 +
>  src/app/controllers/deployables_controller.rb      |   30 ++----
>  src/app/controllers/deployments_controller.rb      |   86 ++++++++++--------
>  src/app/controllers/images_controller.rb           |   35 ++++---
>  src/app/controllers/pool_families_controller.rb    |    2 +-
>  src/app/models/deployable.rb                       |   61 +++++++------
>  src/app/models/pool_family.rb                      |    7 ++
>  src/app/models/provider_account.rb                 |   10 ++-
>  .../views/api/environments/_environment.xml.haml   |    9 ++
>  src/app/views/api/environments/index.xml.haml      |    4 +
>  src/app/views/api/environments/show.xml.haml       |    2 +
>  src/app/views/api/images/_image.xml.haml           |    1 +
>  src/app/views/deployables/_image_uuids.html.haml   |    8 +-
>  src/app/views/deployables/show.html.haml           |    8 +-
>  .../deployments/launch_from_catalog.html.haml      |    2 +-
>  src/app/views/images/_list.html.haml               |   47 ++++++----
>  src/app/views/images/import.html.haml              |    3 +
>  src/app/views/images/show.html.haml                |   23 +++--
>  src/config/locales/en.yml                          |    8 ++
>  src/config/routes.rb                               |    4 +
>  src/features/deployables.feature                   |    1 +
>  src/features/deployment.feature                    |    3 +-
>  src/lib/image.rb                                   |    6 +-
>  src/spec/controllers/api/images_controller_spec.rb |   26 +++++-
>  .../api/provider_images_controller_spec.rb         |    5 +-
>  .../controllers/deployables_controller_spec.rb     |   12 ++--
>  .../controllers/deployments_controller_spec.rb     |    5 +-
>  src/spec/vcr/cassettes/iwhd_connection.yml         |   96 ++++++++++++++++++++
>  30 files changed, 439 insertions(+), 166 deletions(-)
>  create mode 100644 src/app/controllers/api/environments_controller.rb
>  create mode 100644 src/app/views/api/environments/_environment.xml.haml
>  create mode 100644 src/app/views/api/environments/index.xml.haml
>  create mode 100644 src/app/views/api/environments/show.xml.haml
> 
> diff --git a/src/app/controllers/api/environments_controller.rb b/src/app/controllers/api/environments_controller.rb
> new file mode 100644
> index 0000000..9260caa
> --- /dev/null
> +++ b/src/app/controllers/api/environments_controller.rb
> @@ -0,0 +1,39 @@
> +#
> +#   Copyright 2012 Red Hat, Inc.
> +#
> +#   Licensed 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.
> +#
> +
> +# Filters added to this controller apply to all controllers in the application.
> +# Likewise, all the methods added will be available for all controllers.
> +
> +module Api
> +  class EnvironmentsController < ApplicationController
> +    before_filter :require_user
> +
> +    respond_to :xml
> +    layout :false
> +
> +    def index
> +      @environments = PoolFamily.all
> +    end
> +
> +    def show
> +      begin
> +        @environment = PoolFamily.find(params[:id])
> +      rescue ActiveRecord::RecordNotFound
> +        @environment = PoolFamily.find_by_name(params[:id])
> +      end
> +    end
> +  end
> +end

In general, I'm not crazy about having separate api/ controllers -- the
functionality should be in with our existing controllers. I suspect that
this one needs to be the way it is due to existing assumptions, but if
it doesn't, can we merge this in with the existing PoolFamilies
controller?

> diff --git a/src/app/controllers/api/images_controller.rb b/src/app/controllers/api/images_controller.rb
> index 18870f2..465a20b 100644
> --- a/src/app/controllers/api/images_controller.rb
> +++ b/src/app/controllers/api/images_controller.rb
> @@ -20,13 +20,16 @@
>  module Api
>    class ImagesController < ApplicationController
>      before_filter :require_user_api
> -    before_filter :check_permissions, :only => [:create, :destroy]
>  
>      respond_to :xml
>      layout :false
>  
>      def index
> -      @images = Aeolus::Image::Warehouse::Image.all
> +      if (@environment = params[:environment_id])
> +        @images = Aeolus::Image::Warehouse::Image.by_environment(@environment)
> +      else
> +        @images = Aeolus::Image::Warehouse::Image.all
> +      end
>        respond_with(@images)
>      end
>  
> @@ -47,9 +50,16 @@ module Api
>        begin
>          if req[:type] == :failed
>            raise(Aeolus::Conductor::API::InsufficientParametersSupplied.new(400, t("api.error_messages.specify_a_type_build_or_import")))
> +        elsif req[:type] == :failed_env
> +          raise(Aeolus::Conductor::API::InsufficientParametersSupplied.new(400, t("api.error_messages.specify_environment")))
>          elsif req[:type] == :build
>            errors = TemplateXML.validate(req[:params][:template])
>            raise Aeolus::Conductor::API::ParameterDataIncorrect.new(400, t("api.error_messages.invalid_template", :errors => errors.join(", "))) if errors.any?
> +          raise Aeolus::Conductor::API::InsufficientParametersSupplied.new(400, t("api.error_messages.environment_required")) if req[:params][:environment].nil?
> +          @pool_family = PoolFamily.find_by_name(req[:params][:environment])
> +          raise Aeolus::Conductor::API::ParameterDataIncorrect.new(400, t("api.error_messages.environment_not_found", :environment => req[:params][:environment])) if @pool_family.nil?
> +          check_permissions
> +          environment_targets = @pool_family.build_targets
>  
>            @targetnotfound=false
>            @badtarget=""
> @@ -58,20 +68,46 @@ module Api
>              if !target
>                @targetnotfound=true
>                @badtarget=t
> +            elsif !environment_targets.include?(t)
> +              raise(Aeolus::Conductor::API::ParameterDataIncorrect.new(400, t("api.error_messages.target_not_found_in_environment", :target => t, :targets => environment_targets.join(", "))))
>              end
>            end
>            if @targetnotfound
>              raise(Aeolus::Conductor::API::TargetNotFound.new(404, t("api.error_messages.target_not_found", :target => @badtarget)))
>            end
> -          @image = Aeolus::Image::Factory::Image.new(req[:params])
> +
> +          uuid = UUIDTools::UUID.timestamp_create.to_s

I'm not sure if this actually matters, but as were discussion on IRC,
UUID.timestamp_create doesn't create particularly-random UUIDs:

>> 5.times {puts UUIDTools::UUID.timestamp_create.to_s}
94dc0942-5821-11e1-9477-70f395039857
94dc13f6-5821-11e1-9477-70f395039857
94dc195a-5821-11e1-9477-70f395039857
94dc1ed2-5821-11e1-9477-70f395039857
94dc260c-5821-11e1-9477-70f395039857

Since it's timestamp based and we're not creating a barrage of UUIDs as
fast as Ruby can create them as in my example, I don't think is a real
issue. I do wonder if we can use entirely random ones, though.

> +          @tpl = Aeolus::Image::Warehouse::Template.create!(uuid, req[:params][:template], {
> +            :object_type => 'template',
> +            :uuid => uuid
> +          })
> +          uuid = UUIDTools::UUID.timestamp_create.to_s
> +          body = "<image><name>#{@tpl.name}</name></image>"
> +          iwhd_image = Aeolus::Image::Warehouse::Image.create!(uuid, body, {
> +            :uuid => uuid,
> +            :object_type => 'image',
> +            :template => @tpl.uuid,
> +            :environment => @pool_family.name
> +          })
> +          @image = Aeolus::Image::Factory::Image.new(:id => iwhd_image.id)
> +          @image.targets = req[:params][:targets]
> +          @image.template = req[:params][:template]
>            @image.save!
>            respond_with(@image)
>          elsif req[:type] == :import
>            account = ProviderAccount.find_by_label(req[:params][:provider_account_name])
>            raise(Aeolus::Conductor::API::ProviderAccountNotFound.new(404, t("api.error_messages.provider_account_not_found",
>              :name => req[:params][:provider_account_name]))) unless account.present?
> +          raise Aeolus::Conductor::API::InsufficientParametersSupplied.new(400, t("api.error_messages.environment_required")) if req[:params][:environment].nil?
> +          @pool_family = PoolFamily.find_by_name(req[:params][:environment])
> +          raise Aeolus::Conductor::API::ParameterDataIncorrect.new(400, t("api.error_messages.environment_not_found", :environment => req[:params][:environment])) if @pool_family.nil?
> +          check_permissions
> +          environment_accounts = @pool_family.provider_accounts
> +          if !environment_accounts.include?(account)
> +            raise(Aeolus::Conductor::API::ParameterDataIncorrect.new(400, t("api.error_messages.account_not_found_in_environment", :account => account.label, :accounts => environment_accounts.collect{|a|a.label}.join(", "))))
> +          end
>            begin
> -            @image = Image.import(account, req[:params][:target_identifier], req[:params][:image_descriptor])
> +            @image = Image.import(account, req[:params][:target_identifier], @pool_family, req[:params][:image_descriptor])
>            rescue Aeolus::Conductor::Base::ImageNotFound
>              raise(Aeolus::Conductor::API::ImageNotFound.new(404, t("api.error_messages.image_not_found_on_provider",
>                :image => req[:params][:target_identifier])))
> @@ -86,6 +122,8 @@ module Api
>      def destroy
>        begin
>          if @image = Aeolus::Image::Warehouse::Image.find(params[:id])
> +          @pool_family = PoolFamily.find_by_name(@image.environment)
> +          check_permissions
>            @provider_images = @image.provider_images
>            if @image.delete!
>              respond_with(@image, @provider_images)
> @@ -103,17 +141,21 @@ module Api
>      private
>      def process_post(body)
>        doc = Nokogiri::XML CGI.unescapeHTML(body)
> -      if !doc.xpath("/image/targets").empty? && !doc.xpath("/image/tdl/template").empty?
> +      if !doc.xpath("/image/targets").empty? && !doc.xpath("/image/tdl/template").empty? && !doc.xpath("/image/environment").empty?
>          { :type => :build, :params => { :template => doc.xpath("/image/tdl/template").to_s,
> -                                        :targets => doc.xpath("/image/targets").text }
> +                                        :targets => doc.xpath("/image/targets").text,
> +                                        :environment => doc.xpath("/image/environment").text}
>          }
>        elsif !doc.xpath("/image/provider_account_name").empty? && !doc.xpath("/image/target_identifier").empty? &&
> -                 !doc.xpath("/image/image_descriptor").empty?
> +                 !doc.xpath("/image/image_descriptor").empty? && !doc.xpath("/image/environment").empty?
>  
>          { :type => :import, :params => { :target_identifier => doc.xpath("/image/target_identifier").text,
>                                           :image_descriptor => doc.xpath("/image/image_descriptor").children.first.to_s,
> -                                         :provider_account_name => doc.xpath("/image/provider_account_name").text }
> +                                         :provider_account_name => doc.xpath("/image/provider_account_name").text,
> +                                        :environment => doc.xpath("/image/environment").text }
>          }
> +      elsif !doc.xpath("/image").empty? && doc.xpath("/image/environment").empty?
> +        { :type => :failed_env }
>        else
>          { :type => :failed }
>        end
> diff --git a/src/app/controllers/api/provider_images_controller.rb b/src/app/controllers/api/provider_images_controller.rb
> index 7055cb4..ec98813 100644
> --- a/src/app/controllers/api/provider_images_controller.rb
> +++ b/src/app/controllers/api/provider_images_controller.rb
> @@ -64,12 +64,16 @@ module Api
>          return
>        end
>  
> +      @pool_family = PoolFamily.find_by_name(target_images.first.build.image.environment)
> +      environment_accounts = @pool_family.provider_accounts
>        @provider_images = []
>        @errors = []
>        doc.xpath("/provider_image/provider_account").text.split(",").each do |account_name|
>          account = ProviderAccount.find_by_label(account_name)
>          if !account
>            raise(Aeolus::Conductor::API::ProviderAccountNotFound.new(404, t("api.error_messages.provider_account_not_found", :name => account_name)))
> +        elsif !environment_accounts.include?(account)
> +          raise(Aeolus::Conductor::API::ParameterDataIncorrect.new(400, t("api.error_messages.account_not_found_in_environment", :account => account.label, :accounts => environment_accounts.collect{|a|a.label}.join(", "))))
>          end
>  
>          target_image = find_target_image_for_account(target_images, account)
> diff --git a/src/app/controllers/deployables_controller.rb b/src/app/controllers/deployables_controller.rb
> index 4e4404f..e37bc4a 100644
> --- a/src/app/controllers/deployables_controller.rb
> +++ b/src/app/controllers/deployables_controller.rb
> @@ -63,7 +63,10 @@ class DeployablesController < ApplicationController
>      require_privilege(Privilege::VIEW, @deployable)
>      save_breadcrumb(polymorphic_path([@catalog, @deployable]), @deployable.name)
>      @providers = Provider.all
> -    @catalogs_options = Catalog.list_for_user(current_user, Privilege::VIEW).select {|c| !@deployable.catalogs.include?(c)}
> +    @catalogs_options = Catalog.list_for_user(current_user, Privilege::VIEW).select do |c|
> +      !@deployable.catalogs.include?(c) and
> +        @deployable.catalogs.first.pool.pool_family == c.pool.pool_family
> +    end
>  
>      if @catalog.present?
>        add_permissions_inline(@deployable, '', {:catalog_id => @catalog.id})
> @@ -71,28 +74,14 @@ class DeployablesController < ApplicationController
>        add_permissions_inline(@deployable)
>      end
>  
> -    @images_details = @deployable.get_image_details
> -    images = @deployable.fetch_images
> -    uuids = @deployable.fetch_image_uuids
> -    @missing_images = images.zip(uuids).select{|p| p.first.nil?}.map{|p| p.second}
> -    @images_hash_details = images != [nil] ? @deployable.get_uuids_hash(images) : nil
> -
> -    @images_details.each do |assembly|
> -      assembly.keys.each do |key|
> -        @deployable_errors ||= []
> -        @deployable_errors << "#{assembly[:name]}: #{assembly[key]}" if key.to_s =~ /^error\w+/
> -      end
> -      if @missing_images.include?(assembly[:image_uuid])
> -        @deployable_errors << "#{assembly[:name]}: Image (UUID: #{assembly[:image_uuid]}) doesn't exist."
> -      end
> -      flash.now[:error] = @deployable_errors unless @deployable_errors.empty?
> -    end
> +    @images_details, images, @missing_images, @deployable_errors = @deployable.get_image_details
> +    flash.now[:error] = @deployable_errors unless @deployable_errors.empty?
>  
>      return unless @missing_images.empty?
>  
>      @build_results = {}
>      @pushed_count = 0
> -    ProviderAccount.includes(:provider).where('providers.enabled' => true).each do |account|
> +    ProviderAccount.includes(:provider, :pool_families).where('providers.enabled' => true, 'pool_families.id' => @deployable.catalogs.first.pool.pool_family.id).each do |account|
>        type = account.provider.provider_type.deltacloud_driver
>        @build_results[type] ||= []
>        status = @deployable.build_status(images, account)
> @@ -163,8 +152,8 @@ class DeployablesController < ApplicationController
>      rescue => e
>        flash.now[:warning]= t('deployables.flash.warning.failed', :message => e.message) if @deployable.errors.empty?
>        if params[:create_from_image].present?
> -        load_catalogs
>          @image = Aeolus::Image::Warehouse::Image.find(params[:create_from_image])
> +        load_catalogs
>          @hw_profiles = HardwareProfile.frontend.list_for_user(current_user, Privilege::VIEW)
>        else
>          @catalog = @selected_catalogs.first
> @@ -267,7 +256,8 @@ class DeployablesController < ApplicationController
>    end
>  
>    def load_catalogs
> -    @catalogs = Catalog.list_for_user(current_user, Privilege::MODIFY)
> +    @pool_family = PoolFamily.where(:name => @image.environment).first
> +    @catalogs = Catalog.includes(:pool).list_for_user(current_user, Privilege::MODIFY).where('pools.pool_family_id' => @pool_family.id)
>    end
>  
>    def import_xml_from_url(url)
> diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb
> index b858723..e36c483 100644
> --- a/src/app/controllers/deployments_controller.rb
> +++ b/src/app/controllers/deployments_controller.rb
> @@ -61,14 +61,15 @@ class DeploymentsController < ApplicationController
>        redirect_to launch_new_deployments_path(:pool_id => params[:deployment][:pool_id]) and return
>      end
>  
> -    @deployable = Deployable.find(params[:deployable_id])
>      @deployment = Deployment.new(params[:deployment])
>      @pool = @deployment.pool
> +    init_new_deployment_attrs
>      require_privilege(Privilege::CREATE, Deployment, @pool)
>      require_privilege(Privilege::USE, @deployable)
> -    init_new_deployment_attrs
> +    img, img2, missing, d_errors = @deployable.get_image_details
> +    flash[:error] = d_errors unless d_errors.empty?
>  
> -    unless @deployable_xml && @deployment.valid_deployable_xml?(@deployable_xml)
> +    unless @deployable && @deployable.xml && @deployment.valid_deployable_xml?(@deployable.xml) && d_errors.empty?
>        render 'launch_new' and return
>      end
>  
> @@ -84,16 +85,17 @@ class DeploymentsController < ApplicationController
>    end
>  
>    def overview
> -    @deployable = Deployable.find(params[:deployable_id])
>      @deployment = Deployment.new(params[:deployment])
>      @pool = @deployment.pool
> +    init_new_deployment_attrs
>      require_privilege(Privilege::CREATE, Deployment, @pool)
>      require_privilege(Privilege::USE, @deployable)
> -    init_new_deployment_attrs
>      @launch_parameters_encoded = Base64.encode64(ActiveSupport::JSON.encode(@deployment.launch_parameters))
> +    img, img2, missing, d_errors = @deployable.get_image_details
> +    flash[:error] = d_errors unless d_errors.empty?
>  
>      respond_to do |format|
> -      if @deployable_xml && @deployment.valid_deployable_xml?(@deployable_xml)
> +      if @deployable.xml && @deployment.valid_deployable_xml?(@deployable.xml) && d_errors.empty?
>          @errors = @deployment.check_assemblies_matches(current_user)
>          set_errors_flash(@errors)
>          @additional_quota = count_additional_quota(@deployment)
> @@ -119,44 +121,54 @@ class DeploymentsController < ApplicationController
>      @pool = @deployment.pool
>      require_privilege(Privilege::CREATE, Deployment, @pool)
>      init_new_deployment_attrs
> -    @deployment.deployable_xml = @deployable_xml if @deployable_xml
> +    @deployment.deployable_xml = @deployable.xml if @deployable && @deployable.xml
>      if params.has_key?(:deployable_id)
> -      @deployable = Deployable.find(params[:deployable_id])
>        require_privilege(Privilege::USE, @deployable)
>      end
> -    @deployment.owner = current_user
> -    load_assemblies_services
> -    if params.delete(:commit) == 'back'
> -      view = launch_parameters_encoded.blank? ? 'launch_new' : 'launch_time_params'
> -      render view and return
> -    end
> -
> +    img, img2, missing, d_errors = @deployable.get_image_details
>      respond_to do |format|
> -      if @deployment.save
> -        status = @deployment.launch(current_user)
> -        if status[:errors].empty?
> -          flash[:notice] = t "deployments.flash.notice.launched"
> -        else
> -          flash[:error] = {
> -              :summary  => t("deployments.flash.error.failed_to_launch_assemblies"),
> -              :failures => status[:errors],
> -              :successes => status[:successes]
> -          }
> -        end
> -        format.html { redirect_to deployment_path(@deployment) }
> -        format.js do
> -          @deployment_properties = @deployment.properties
> -          render :partial => 'properties'
> -        end
> -        format.json { render :json => @deployment, :status => :created }
> -      else
> -        # We need @pool to re-display the form
> +      unless d_errors.empty?

So this isn't really "wrong" in any sense, but "if d_errors.present?"
seems less like a double-negative to me, compared to "unless
d_errors.empty?". I imagine that's entirely personal preference, though,
so don't feel compelled to do anything here unless you agree.

>          @pool = @deployment.pool
>          flash.now[:warning] = t "deployments.flash.warning.failed_to_launch"
> -        init_new_deployment_attrs
> +        flash[:error] = d_errors
>          format.html { render :action => 'overview' }
>          format.js { launch_new }
> -        format.json { render :json => @deployment.errors, :status => :unprocessable_entity }
> +        format.json { render :json => d_errors, :status => :unprocessable_entity }
> +      else
> +
> +        @deployment.owner = current_user
> +        load_assemblies_services
> +        if params.delete(:commit) == 'back'
> +          view = launch_parameters_encoded.blank? ? 'launch_new' : 'launch_time_params'
> +          render view and return
> +        end
> +
> +        if @deployment.save
> +          status = @deployment.launch(current_user)
> +          if status[:errors].empty?
> +            flash[:notice] = t "deployments.flash.notice.launched"
> +          else
> +            flash[:error] = {
> +              :summary  => t("deployments.flash.error.failed_to_launch_assemblies"),
> +              :failures => status[:errors],
> +              :successes => status[:successes]
> +            }
> +          end
> +          format.html { redirect_to deployment_path(@deployment) }
> +          format.js do
> +            @deployment_properties = @deployment.properties
> +            render :partial => 'properties'
> +          end
> +          format.json { render :json => @deployment, :status => :created }
> +        else
> +          # We need @pool to re-display the form
> +          @pool = @deployment.pool
> +          flash.now[:warning] = t "deployments.flash.warning.failed_to_launch"
> +          init_new_deployment_attrs
> +          format.html { render :action => 'overview' }
> +          format.js { launch_new }
> +          format.json { render :json => @deployment.errors, :status => :unprocessable_entity }
> +        end
>        end
>      end
>    end
> @@ -372,7 +384,7 @@ class DeploymentsController < ApplicationController
>    def init_new_deployment_attrs
>      @deployables = Deployable.list_for_user(current_user, Privilege::USE).select{|d| d.catalogs.collect {|c| c.pool}.include?(@pool)}
>      @pools = Pool.list_for_user(current_user, Privilege::CREATE, Deployment)
> -    @deployable_xml = params[:deployable_id] ? Deployable.find(params[:deployable_id]).xml : nil
> +    @deployable = params[:deployable_id] ? Deployable.find(params[:deployable_id]) : nil
>      @realms = FrontendRealm.all
>      @hardware_profiles = HardwareProfile.all(
>          :include => :architecture,
> diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb
> index 41108c5..cc2231e 100644
> --- a/src/app/controllers/images_controller.rb
> +++ b/src/app/controllers/images_controller.rb
> @@ -21,8 +21,8 @@ class ImagesController < ApplicationController
>    def index
>      set_admin_environments_tabs 'images'
>      @header = [
> -      { :name => 'checkbox', :class => 'checkbox', :sortable => false },
>        { :name => t('images.index.name'), :sort_attr => :name },
> +      { :name => t('images.environment_header'), :sort_attr => :name },
>        { :name => t('images.index.os'), :sort_attr => :name },
>        { :name => t('images.index.os_version'), :sort_attr => :name },
>        { :name => t('images.index.architecture'), :sort_attr => :name },
> @@ -37,6 +37,7 @@ class ImagesController < ApplicationController
>  
>    def show
>      @image = Aeolus::Image::Warehouse::Image.find(params[:id])
> +    @environment = PoolFamily.where('name' => @image.environment).first
>      if @image.imported?
>        begin
>          # For an imported image, we only want to show the actual provider account
> @@ -46,12 +47,12 @@ class ImagesController < ApplicationController
>          type = provider.provider_type
>          acct = ProviderAccount.enabled.find_by_provider_name_and_login(provider.name, pimg.provider_account_identifier)
>          raise unless acct
> -        @account_groups = {type.deltacloud_driver => {:type => type, :accounts => [acct]}}
> +        @account_groups = {type.deltacloud_driver => {:type => type, :accounts => [[acct, at environment.provider_accounts.include?(acct)]]}}
>        rescue Exception => e
> -        @account_groups = ProviderAccount.enabled.group_by_type(current_user)
> +        @account_groups = ProviderAccount.enabled.group_by_type(@environment)
>        end
>      else
> -      @account_groups = ProviderAccount.enabled.group_by_type(current_user)
> +      @account_groups = ProviderAccount.enabled.group_by_type(@environment)
>      end
>      # according to imagefactory Builder.first shouldn't be implemented yet
>      # but it does what we need - returns builder object which contains
> @@ -74,8 +75,8 @@ class ImagesController < ApplicationController
>          active_pushes = @account_groups.inject({})  do |result, (driver, group)|
>            timg = @target_images_by_target[driver]
>            group[:accounts].each do |account|
> -            result[account.id] = @builder.find_active_push(timg.id, account.provider.name, account.credentials_hash['username'])
> -            result[account.id].attributes['status'].capitalize! if result[account.id]
> +            result[account[0].id] = @builder.find_active_push(timg.id, account[0].provider.name, account.credentials_hash['username'])

I'll comment more in a second, but result[account[0].id] kind of makes
me wince on account of the fact that it's hard to figure out what in the
world it represents. This goes for a lot of the names in this section.

> +            result[account[0].id].attributes['status'].capitalize! if result[account[0].id]
>            end if timg.present?
>  
>            result
> @@ -84,7 +85,7 @@ class ImagesController < ApplicationController
>          provider_images = @account_groups.inject({})  do |result, (driver, group)|
>            timg = @target_images_by_target[driver]
>            group[:accounts].each do |account|
> -            result[account.id] = timg.find_provider_image_by_provider_and_account(account.provider.name, account.credentials_hash['username']).first
> +            result[account[0].id] = timg.find_provider_image_by_provider_and_account(account[0].provider.name, account[0].credentials_hash['username']).first
>            end if timg.present?
>  
>            result
> @@ -98,7 +99,7 @@ class ImagesController < ApplicationController
>          failed_push_counts = @account_groups.inject({})  do |result, (driver, group)|
>            timg = @target_images_by_target[driver]
>            group[:accounts].each do |account|
> -            result[account.id] = @builder.failed_push_count(timg.id, account.provider.name, account.credentials_hash['username'])
> +            result[account[0].id] = @builder.failed_push_count(timg.id, account[0].provider.name, account[0].credentials_hash['username'])
>            end if timg.present?
>  
>            result
> @@ -120,7 +121,8 @@ class ImagesController < ApplicationController
>  
>    def rebuild_all
>      @image = Aeolus::Image::Warehouse::Image.find(params[:id])
> -    targets = Provider.enabled.list_for_user(current_user, Privilege::VIEW).map {|p| p.provider_type.deltacloud_driver}.uniq
> +    @environment = PoolFamily.where('name' => @image.environment).first
> +    targets = @environment.build_targets
>      unless targets.empty?
>        factory_image = Aeolus::Image::Factory::Image.new(:id => @image.id)
>        factory_image.targets = targets.join(',')
> @@ -132,13 +134,14 @@ class ImagesController < ApplicationController
>  
>    def push_all
>      @image = Aeolus::Image::Warehouse::Image.find(params[:id])
> +    @environment = PoolFamily.where('name' => @image.environment).first
>      @build = Aeolus::Image::Warehouse::ImageBuild.find(params[:build_id])
>      # only latest builds can be pushed
>      unless latest_build?(@build)
>        redirect_to image_path(@image.id)
>        return
>      end
> -    accounts = ProviderAccount.list_for_user(current_user, Privilege::VIEW)
> +    accounts = @environment.provider_accounts
>      target_images = @build.target_images
>      accounts.each do |account|
>        if account.image_status(@image) == :not_pushed
> @@ -169,20 +172,21 @@ class ImagesController < ApplicationController
>    end
>  
>    def new
> +    @environment = PoolFamily.find(params[:environment])
>      if 'import' == params[:tab]
> -      @accounts = ProviderAccount.enabled.list_for_user(current_user, Privilege::USE)
> +      @accounts = @environment.provider_accounts.enabled.list_for_user(current_user, Privilege::USE)
>        render :import and return
> -    else
> -      @environment = PoolFamily.find(params[:environment])
>      end
>  
>    end
>  
>    def import
>      account = ProviderAccount.find(params[:provider_account])
> +    @environment = PoolFamily.find(params[:environment])
> +
>      xml = "<image><name>#{params[:name]}</name></image>" unless params[:name].blank?
>      begin
> -      image = Image.import(account, params[:image_id], xml)
> +      image = Image.import(account, params[:image_id], @environment, xml)
>        flash[:success] = t("images.import.image_imported")
>        redirect_to image_url(image.id) and return
>      rescue Exception => e
> @@ -283,7 +287,8 @@ class ImagesController < ApplicationController
>      @image = Aeolus::Image::Warehouse::Image.create!(uuid, body, {
>        :uuid => uuid,
>        :object_type => 'image',
> -      :template => @tpl.uuid
> +      :template => @tpl.uuid,
> +      :environment => @environment.name
>      })
>      flash.now[:error] = t('images.flash.notice.created')
>      redirect_to image_path(@image.id)
> diff --git a/src/app/controllers/pool_families_controller.rb b/src/app/controllers/pool_families_controller.rb
> index 98322eb..d9398ed 100644
> --- a/src/app/controllers/pool_families_controller.rb
> +++ b/src/app/controllers/pool_families_controller.rb
> @@ -74,7 +74,7 @@ class PoolFamiliesController < ApplicationController
>      @pool_family = PoolFamily.find(params[:id])
>      save_breadcrumb(pool_family_path(@pool_family), @pool_family.name)
>      require_privilege(Privilege::VIEW, @pool_family)
> -    @images = Aeolus::Image::Warehouse::Image.all
> +    @images = Aeolus::Image::Warehouse::Image.by_environment(@pool_family.name)
>      load_pool_family_tabs
>  
>      respond_to do |format|
> diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
> index c9df4c5..6d33eef 100644
> --- a/src/app/models/deployable.rb
> +++ b/src/app/models/deployable.rb
> @@ -117,28 +117,49 @@ class Deployable < ActiveRecord::Base
>    #get details of image for deployable#show
>    def get_image_details
>      deployable_xml = DeployableXML.new(xml)
> +    uuids = deployable_xml.image_uuids
> +    images = []
> +    missing_images = []
>      assemblies_array ||= []
> +    deployable_errors ||= []
>      deployable_xml.assemblies.each do |assembly|
> -      assembly_hash ||= {:name => assembly.name}
> +      assembly_hash = {}
> +      image = Aeolus::Image::Warehouse::Image.find(assembly.image_id)
> +      if image.nil?
> +        missing_images << assembly.image_id
> +        deployable_errors << I18n.t("deployables.flash.error.missing_image",
> +                                    :assembly => assembly.name,
> +                                    :uuid => assembly.image_id)
> +      else
> +        if image.environment != catalogs.first.pool.pool_family.name
> +          deployable_errors << I18n.t("deployables.flash.error.wrong_environment",
> +                                      :assembly => assembly.name,
> +                                      :uuid => assembly.image_id,
> +                                      :wrong_env => image.environment,
> +                                      :environment => catalogs.first.pool.pool_family.name)
> +        end
> +        images << image
> +        assembly_hash[:build_and_target_uuids] = get_build_and_target_uuids(image)
> +      end
> +      assembly_hash[:name] = assembly.name
>        assembly_hash[:image_uuid] = assembly.image_id
>        assembly_hash[:images_count] = assembly.images_count
>        if assembly.hwp
> -        hwp_name = assembly.hwp
> -        hwp = HardwareProfile.find_by_name(hwp_name)
> +        hwp = HardwareProfile.find_by_name(assembly.hwp)
>          if hwp
>            assembly_hash[:hwp_name] = hwp.name
>            assembly_hash[:hwp_hdd] = hwp.storage.value
>            assembly_hash[:hwp_ram] = hwp.memory.value
>            assembly_hash[:hwp_arch] = hwp.architecture.value
>          else
> -          assembly_hash[:error_hwp] = I18n.t('deployables.error.hwp_not_exists', :name => hwp_name)
> +          deployable_errors << "#{assembly_hash[:name]}: " + I18n.t('deployables.error.hwp_not_exists', :name => assembly.hwp)
>          end
>        else
> -        assembly_hash[:error_hwp] = I18n.t('deployables.error.attribute_not_exist')
> +        deployable_errors << "#{assembly_hash[:name]}: " + I18n.t('deployables.error.attribute_not_exist')
>        end
>        assemblies_array << assembly_hash
>      end
> -    assemblies_array
> +    [assemblies_array, images, missing_images, deployable_errors]

I'm really not crazy about this return value, although I confess to
having done similar things at times myself. The code that references the
return result just gets kind of hard to read -- what is foo[3] and why
are we putting it in flash[:error], yet iterating over foo[1]? It just
sort of feels like magic numbers, applied to arrays.

I think I'd be happier if it were a hash, personally:

 {:assemblies => assemblies_array, :images => images,
  :missing_images => missing_images,
  :deployable_errors => deployable_errors}

In that case, it's clear what foo[:deployable_errors] represents, and a
developer that comes along later is far less-likely to accidentally
choose the wrong element when working with the output -- the difference
between foo[2] and foo[3] wouldn't be apparent, but foo [:images] and
foo[:missing_images] are pretty obvious in purpose.

However, I admit that I wouldn't NACK a patch over this, so if this
turns into a sizable rewrite, I could look the other way.

>    end
>  
>    def build_status(images, account)
> @@ -156,29 +177,11 @@ class Deployable < ActiveRecord::Base
>      [catalog, self]
>    end
>  
> -  def get_uuids_hash(images)
> -    image_uuids = []
> -    target_image_uuids = []
> -    latest_build_uuid = []
> -
> -    images.each do |i|
> -      image_uuids << (i.respond_to?(:uuid) ? i.uuid : nil)
> -      latest_build = i.latest_pushed_build if i.respond_to?(:latest_pushed_build)
> -      latest_build_uuid << (latest_build ? latest_build.uuid : nil)
> -      if latest_build
> -        target_images = latest_build.target_images
> -        target_image_uuids_for_build = []
> -        target_images.each do |ti|
> -          target_image_uuids_for_build << ti.uuid
> -        end
> -        target_image_uuids << target_image_uuids_for_build
> -      else
> -        target_image_uuids << nil
> -      end
> -    end
> -
> -  #return array [[image_uuid, latest_build_uuid, [target_image_uuids]], [..]]
> -  image_uuids.zip(latest_build_uuid, target_image_uuids)
> +  def get_build_and_target_uuids(image)
> +    latest_build = image.respond_to?(:latest_pushed_build) ? image.latest_pushed_build : nil
> +    [(image.respond_to?(:uuid) ? image.uuid : nil),
> +     (latest_build ? latest_build.uuid : nil),
> +     (latest_build ? latest_build.target_images.collect { |ti| ti.uuid} : nil)]
>    end
>  
>    private
> diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb
> index 9bb54b7..3e834a0 100644
> --- a/src/app/models/pool_family.rb
> +++ b/src/app/models/pool_family.rb
> @@ -83,4 +83,11 @@ class PoolFamily < ActiveRecord::Base
>        :available_quota => avail,
>      }
>    end
> +  def build_targets
> +    targets = []
> +    ProviderAccount.enabled.group_by_type(self).each do |driver, group|
> +      targets << driver if group[:included]
> +    end
> +    targets
> +  end
>  end
> diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
> index 251a1e8..aa450eb 100644
> --- a/src/app/models/provider_account.rb
> +++ b/src/app/models/provider_account.rb
> @@ -316,12 +316,16 @@ class ProviderAccount < ActiveRecord::Base
>  
>    PRESET_FILTERS_OPTIONS = []
>  
> -  def self.group_by_type(user)
> +  def self.group_by_type(pool_family)
>      res = {}
> -    ProviderAccount.list_for_user(user, Privilege::VIEW).each do |account|
> +    family_accounts = pool_family.nil? ? [] : pool_family.provider_accounts
> +    ProviderAccount.enabled.each do |account|
>        ptype = account.provider.provider_type
>        res[ptype.deltacloud_driver] ||= {:type => ptype, :accounts => []}
> -      res[ptype.deltacloud_driver][:accounts] << account
> +      res[ptype.deltacloud_driver][:accounts] << [account, family_accounts.include?(account)]
> +    end
> +    res.each do |driver, group|
> +      group[:included] = (group[:accounts].count{|a| a[1]} > 0)
>      end
>      res
>    end
> diff --git a/src/app/views/api/environments/_environment.xml.haml b/src/app/views/api/environments/_environment.xml.haml
> new file mode 100644
> index 0000000..5fd425f
> --- /dev/null
> +++ b/src/app/views/api/environments/_environment.xml.haml
> @@ -0,0 +1,9 @@
> +!!!XML
> +%environment{:id => environment.id, :href => api_image_url(environment.id) }
> +  %name= environment.name
> +  %description= environment.description
> +  %targets{:type => 'xs:list'}
> +    - ProviderAccount.enabled.group_by_type(environment).select{|driver,group| group[:included]}.each do |driver, group|
> +      %target{:name => driver}
> +        - group[:accounts].select{|account| account[1]==true}.each do |account|
> +          %account{:name => account[0].name}
> diff --git a/src/app/views/api/environments/index.xml.haml b/src/app/views/api/environments/index.xml.haml
> new file mode 100644
> index 0000000..a3ff564
> --- /dev/null
> +++ b/src/app/views/api/environments/index.xml.haml
> @@ -0,0 +1,4 @@
> +!!!XML
> +%environments
> +  - @environments.each do |env|
> +    = render :partial => 'environment', :locals => {:environment => env}
> diff --git a/src/app/views/api/environments/show.xml.haml b/src/app/views/api/environments/show.xml.haml
> new file mode 100644
> index 0000000..21e5fac
> --- /dev/null
> +++ b/src/app/views/api/environments/show.xml.haml
> @@ -0,0 +1,2 @@
> +!!! XML
> += render :partial => "environment", :locals => {:environment => @environment}
> diff --git a/src/app/views/api/images/_image.xml.haml b/src/app/views/api/images/_image.xml.haml
> index f19b1e6..bfabbde 100644
> --- a/src/app/views/api/images/_image.xml.haml
> +++ b/src/app/views/api/images/_image.xml.haml
> @@ -1,6 +1,7 @@
>  !!!XML
>  %image{:id => image.id, :href => api_image_url(image.id) }
>    %name= image.name
> +  %environment= image.environment
>    %description= image.description
>    %os= image.os.name
>    %os_version= image.os.version
> diff --git a/src/app/views/deployables/_image_uuids.html.haml b/src/app/views/deployables/_image_uuids.html.haml
> index 91902a2..9445641 100644
> --- a/src/app/views/deployables/_image_uuids.html.haml
> +++ b/src/app/views/deployables/_image_uuids.html.haml
> @@ -4,20 +4,20 @@
>        %td
>          %strong="#{t("deployables.show.image_uuid")}"
>        %td
> -        ="#{(@images_hash_details[index][0].nil? ? t('deployables.show.n_a') : @images_hash_details[index][0])}"
> +        ="#{(uuid_arr[0].nil? ? t('deployables.show.n_a') : uuid_arr[0])}"
>      %tr
>        %td
>          %strong="#{t("deployables.show.latest_build_uuid")}"
>        %td
> -        ="#{(@images_hash_details[index][1].nil? ? t("deployables.show.n_a") : @images_hash_details[index][1])}"
> +        ="#{(uuid_arr[1].nil? ? t("deployables.show.n_a") : uuid_arr[1])}"
>      %tr
>        %td
>          %strong="#{t("deployables.show.target_images_uuids")}"
>        %td
> -        - if @images_hash_details[index][2].nil?
> +        - if uuid_arr[2].nil?
>            =t'deployables.show.n_a'
>          - else
>            %ul
> -            - @images_hash_details[index][2].each do |image_uuid|
> +            - uuid_arr[2].each do |image_uuid|
>                %li
>                  =image_uuid
> diff --git a/src/app/views/deployables/show.html.haml b/src/app/views/deployables/show.html.haml
> index 772536f..504016e 100644
> --- a/src/app/views/deployables/show.html.haml
> +++ b/src/app/views/deployables/show.html.haml
> @@ -31,7 +31,7 @@
>            %th.align-center=t'.arch'
>            %th=t(".uuids")
>        %tbody
> -        - @images_details.each_with_index do |assembly, index|
> +        - @images_details.each do |assembly|
>            %tr
>              %td.status= image_valid?(assembly)
>              %td
> @@ -42,16 +42,16 @@
>              %td.align-center=assembly[:hwp_ram]
>              %td.align-center=assembly[:hwp_arch]
>              %td.align-center
> -              - if @images_hash_details.nil?
> +              - if assembly[:build_and_target_uuids].nil?
>                  = t(".n_a")
>                - else
>                  %a.control{:href => '#'}=t(".show_hide_uuids")
> -          - unless @images_hash_details.nil?
> +          - unless assembly[:build_and_target_uuids].nil?
>              %tr.collapsed
>                %td
>                %td{:colspan => 7}
>                  %ul
> -                  = render :partial => "image_uuids", :locals => {:index => index}
> +                  = render :partial => "image_uuids", :locals => {:uuid_arr => assembly[:build_and_target_uuids]}
>  
>  %section.admin-content-section
>    - if @missing_images.empty?
> diff --git a/src/app/views/deployments/launch_from_catalog.html.haml b/src/app/views/deployments/launch_from_catalog.html.haml
> index 8b37b1b..923ba64 100644
> --- a/src/app/views/deployments/launch_from_catalog.html.haml
> +++ b/src/app/views/deployments/launch_from_catalog.html.haml
> @@ -30,7 +30,7 @@
>                  %th=t'.arch'
>                  %th
>                    %strong=t'.deployable_xml'
> -              -deployable.get_image_details.each do |assembly|
> +              -deployable.get_image_details.first.each do |assembly|
>                  %tr
>                    %td
>                      %strong= assembly[:image_uuid]
> diff --git a/src/app/views/images/_list.html.haml b/src/app/views/images/_list.html.haml
> index d75ea59..635297f 100644
> --- a/src/app/views/images/_list.html.haml
> +++ b/src/app/views/images/_list.html.haml
> @@ -1,22 +1,33 @@
>  - content_for :form_header do
> -  - if check_privilege(Privilege::USE, PoolFamily)
> +  - if @pool_family and check_privilege(Privilege::USE, @pool_family)
>      %li= restful_submit_tag t("delete"), "destroy", multi_destroy_images_path, 'DELETE', :id => 'delete_button', :class => 'button danger'
> -    %li= link_to t('images.import.import_image'), new_image_path(:tab => 'import'), { :class => 'button primary', :id => 'import_image_button'}
> -    - if @pool_family
> -      %li= link_to t('images.index.new'), new_image_path(:environment => @pool_family), { :class => 'button primary', :id => 'import_image_button'}
> +    %li= link_to t('images.import.import_image'), new_image_path(:tab => 'import', :environment => @pool_family), { :class => 'button primary', :id => 'import_image_button'}
> +    %li= link_to t('images.index.new'), new_image_path(:environment => @pool_family), { :class => 'button primary', :id => 'import_image_button'}
>  
>  = filter_table(@header, @images) do |image|
> -  %tr{:class => cycle('nostripe','stripe')}
> -    %td{:class => 'checkbox'}
> -      - selected = params[:select] == 'all'
> -      = check_box_tag "images_selected[]", image.id, selected, :id => "image_checkbox_#{image.id}"
> -    %td
> -      = link_to (image.imported? ? image.name + " (Imported)" : image.name), image_path(image.id)
> -    %td
> -      = image.os.name.empty? ? "N/A" : image.os.name
> -    %td
> -      = image.os.version.empty? ? "N/A" : image.os.version
> -    %td
> -      = image.os.arch.empty? ? "N/A" : image.os.arch
> -    %td
> -      = Time.at(image.latest_pushed_or_unpushed_build.timestamp.to_f) rescue ''
> +  - if @pool_family.nil? and image.environment
> +    - environment = PoolFamily.find_by_name(image.environment)
> +  - else
> +    - environment = nil
> +  - if environment.nil? or check_privilege(Privilege::USE, PoolFamily, environment)
> +    %tr{:class => cycle('nostripe','stripe')}
> +      - if @pool_family
> +        %td{:class => 'checkbox'}
> +          - selected = params[:select] == 'all'
> +          = check_box_tag "images_selected[]", image.id, selected, :id => "image_checkbox_#{image.id}"
> +      %td
> +        = link_to (image.imported? ? image.name + " (Imported)" : image.name), image_path(image.id)
> +      - unless @pool_family
> +        %td
> +          - if environment
> +            = link_to environment.name, pool_family_path(environment, :details_tab => 'images')
> +          - else
> +            N/A
> +      %td
> +        = image.os.name.empty? ? "N/A" : image.os.name
> +      %td
> +        = image.os.version.empty? ? "N/A" : image.os.version
> +      %td
> +        = image.os.arch.empty? ? "N/A" : image.os.arch
> +      %td
> +        = Time.at(image.latest_pushed_or_unpushed_build.timestamp.to_f) rescue ''
> diff --git a/src/app/views/images/import.html.haml b/src/app/views/images/import.html.haml
> index e1c759b..f725b74 100644
> --- a/src/app/views/images/import.html.haml
> +++ b/src/app/views/images/import.html.haml
> @@ -2,11 +2,14 @@
>  %header.admin-page-header
>    %h1.images= t('.import_image')
>    #obj_actions.button-container
> +    = t :return_to
> +    = link_to t('images.environment', :environment => @environment.name), pool_family_path(@environment), :class => 'rounded-link'
>  
>  %section.admin-content-section.image-import
>  
>    %section#image-file-form
>      = form_tag(import_images_path, { :multipart => true, :method => :post, :class => :generic }) do
> +      = hidden_field_tag :environment, @environment.id
>        %fieldset
>          %p
>            = label_tag :provider_account, t('.provider_account')
> diff --git a/src/app/views/images/show.html.haml b/src/app/views/images/show.html.haml
> index e26a720..5abb90e 100644
> --- a/src/app/views/images/show.html.haml
> +++ b/src/app/views/images/show.html.haml
> @@ -6,20 +6,23 @@
>      = link_to t('images.index.images'), images_path, :class => 'rounded-link'
>      - if check_privilege(Privilege::USE, PoolFamily)
>        .button-group
> -        = link_to t('.new_deployable_from_image'), new_deployable_path(:create_from_image => @image.id), :class => 'button pill'
> +        - if @environment
> +          = link_to t('.new_deployable_from_image'), new_deployable_path(:create_from_image => @image.id), :class => 'button pill'
>          - unless @image.imported?
>            = link_to t('.template_xml'), template_image_path(@image.uuid), :class => 'button'
>          = button_to t("delete"), image_path(@image.id), :method => 'delete', :confirm => "Are you sure you want to delete?", :class => 'button pill danger', :id => 'delete'
>  
>  %section.admin-content-section
>    %header
> +    %h2=t'properties'
> +    %h3= t('images.environment', :environment => @image.environment)
>      %h2= t('.provider_images')
>      .section-controls
>        = t'.view_build'
>        = form_tag image_path(@image.id), :method => :get do
>          = select_tag :build, options_for_build_select(@builds, @build, @latest_build)
>          = submit_tag t('.select_build'), :id => 'seletect_build_button'
> -      - if check_privilege(Privilege::USE, PoolFamily)
> +      - if check_privilege(Privilege::USE, PoolFamily) and @environment
>          - if @image.imported?
>            = t('.can_not_build_imported_image')
>          -else
> @@ -28,7 +31,7 @@
>              = button_to t('.push_all'), push_all_image_path(@image.id, :build_id => @build.id), :class => 'button pill'
>    .content
>      %ul.image_builds
> -      - @account_groups.each do |driver, group|
> +      - @account_groups.select{|driver,group| group[:included] || @target_images_by_target[driver] || (@build and @builder.find_active_build(@build.id, driver))}.each do |driver, group|
>          - timg = @target_images_by_target[driver]
>          %li
>            %dl
> @@ -62,23 +65,23 @@
>                      = t'.image_uri'
>                    %th.image_controls
>                  %tbody
> -                  - group[:accounts].each do |account|
> -                    - pimg = timg ? timg.find_provider_image_by_provider_and_account(account.provider.name, account.credentials_hash['username']).first : nil
> +                  - group[:accounts].select{|account| account[1]==true || (timg and !timg.find_provider_image_by_provider_and_account(account[0].provider.name, account[0].credentials_hash['username']).empty?)}.each do |account|
> +                    - pimg = timg ? timg.find_provider_image_by_provider_and_account(account[0].provider.name, account[0].credentials_hash['username']).first : nil
>                      %tr
>                        %td
> -                        %strong= account.name
> +                        %strong= account[0].name
>                        %td.light
> -                        = account.provider.name
> +                        = account[0].provider.name
>                        %td.light
>                          = pimg ? pimg.target_identifier : ''
>                        %td.light
>                          = pimg ? pimg.id : ''
>                        %td.image_controls.light
> -                        - if timg and b = @builder.find_active_push(timg.id, account.provider.name, account.credentials_hash['username'])
> +                        - if timg and b = @builder.find_active_push(timg.id, account[0].provider.name, account[0].credentials_hash['username'])
>                            = label_tag b.status
>                          - elsif timg and not pimg and @build and @build.id == @latest_build
> -                          = button_to t('.upload'), image_provider_images_path(@image.id, :build_id => @build.id, :target_image_id => timg.id, :account_id => account.id), :method => :post, :class => 'upload_image'
> -                          - failed_push_count = @builder.failed_push_count(timg.id, account.provider.name, account.credentials_hash['username'])
> +                          = button_to t('.upload'), image_provider_images_path(@image.id, :build_id => @build.id, :target_image_id => timg.id, :account_id => account[0].id), :method => :post, :class => 'upload_image'
> +                          - failed_push_count = @builder.failed_push_count(timg.id, account[0].provider.name, account[0].credentials_hash['username'])
>                            - if failed_push_count > 0
>                              = t('images.show.failed_push_attempts', :count => failed_push_count )
>                          - elsif pimg
> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> index 505be04..117861f 100644
> --- a/src/config/locales/en.yml
> +++ b/src/config/locales/en.yml
> @@ -716,6 +716,7 @@ en:
>          not_found: Target Image not found
>    images:
>      environment: %{environment} Environment
> +    environment_header: Environment
>      import:
>        import_image: Import Image
>        provider_account: Provider Account
> @@ -903,6 +904,8 @@ en:
>          not_selected: "No deployable was not selected!"
>          no_catalog_exists: "No catalog exists! Please create one."
>          no_hwp_exists: "No hardware profile exists! Please create one."
> +        missing_image: "%{assembly}: Image (UUID: %{uuid}) doesn't exist"
> +        wrong_environment: "%{assembly}: Image (UUID: %{uuid}) belongs to the wrong environment (%{wrong_env}) instead of %{environment}"
>        warning:
>          failed: "Deployable was not created: %{message}"
>        notice:
> @@ -1283,7 +1286,10 @@ en:
>        create_new: Create a new Realm Mapping
>    api:
>      error_messages:
> +      account_not_found_in_environment: "Provider Account %{account} is not in this environment. Valid accounts include %{accounts}."
>        build_not_found: Could not find Build %{build}
> +      environment_required: "Environment parameter is required."
> +      environment_not_found: "Environment %{environment} was not found."
>        image_not_found: Could not find Image %{image}
>        image_not_found_on_provider: "Could not find Image %{image} on provider"
>        in_grabbing_target_images: "In grabbing list of target images: %{error}"
> @@ -1298,10 +1304,12 @@ en:
>        provider_image_status_not_found: Could not find status for ProviderImage %{providerimage}
>        push_error: "Could not push TargetImage %{targetimage} to %{account} and error %{error}"
>        specify_a_type_build_or_import: Please specify a type, build or import
> +      specify_environment: Please specify an environment
>        target_image_status_not_found: Could not find status for TargetImage %{targetimage}
>        target_image_not_found: Could not find TargetImage %{targetimage}
>        target_image_not_found_for_account: Could not find an appropriate target image for account %{account}
>        target_not_found: Could not find Target %{target}
> +      target_not_found_in_environment: Target %{target} has no provider accounts in this environment. Valid targets include %{targets}."
>    support:
>      array:
>        words_connector: ', '
> diff --git a/src/config/routes.rb b/src/config/routes.rb
> index 3d440a8..d24f039 100644
> --- a/src/config/routes.rb
> +++ b/src/config/routes.rb
> @@ -262,6 +262,10 @@ Conductor::Application.routes.draw do
>     # :except => [:new, :edit]
>  
>      resources :hooks
> +
> +    resources :environments do
> +      resources :images
> +    end
>    end
>  
>    scope "/api" do
> diff --git a/src/features/deployables.feature b/src/features/deployables.feature
> index a3290cf..f63670c 100644
> --- a/src/features/deployables.feature
> +++ b/src/features/deployables.feature
> @@ -7,6 +7,7 @@ Feature: Manage Catalog Entries
>      Given I am an authorised user
>      And I am logged in
>      And there is mock provider account "mock_account"
> +    And there is a provider account "mock_account" related to pool family "default"
>  
>    Scenario: Create new catalog entry
>      Given there is a "default" catalog
> diff --git a/src/features/deployment.feature b/src/features/deployment.feature
> index 0ef9119..a4e80e8 100644
> --- a/src/features/deployment.feature
> +++ b/src/features/deployment.feature
> @@ -185,9 +185,8 @@ Feature: Manage Deployments
>      When I select "test_catalog_entry" from "deployable_id"
>      When I fill in "deployment_name" with "mynewdeployment"
>      When I press "next_button"
> -    Then I should see "Are you sure you wish to deploy"
>      And I should see an error message
> -    And I should see "front_hwp2 not found."
> +    And I should see "Hardware profile front_hwp2 specified in XML doesn't exist."
>  
>    Scenario: Verify that the launch parameters are displayed
>      Given a pool "mockpool" exists
> diff --git a/src/lib/image.rb b/src/lib/image.rb
> index 02bfdba..b9ce9b7 100644
> --- a/src/lib/image.rb
> +++ b/src/lib/image.rb
> @@ -17,7 +17,7 @@
>  class Image
>    # Given a ProviderAccount and an AMI/image ID on a provider, plus an optional XML string, use aeolus-image
>    # to import the image. Returns an Aeolus::Image::Factory::Image or allows exceptions to bubble up
> -  def self.import(provider_account, image_id, xml=nil)
> +  def self.import(provider_account, image_id, environment, xml=nil)
>      raise Aeolus::Conductor::Base::BlankImageId unless image_id.present?
>      # Verify that the image exists prior to import
>      conn = provider_account.connect
> @@ -27,6 +27,8 @@ class Image
>      xml ||= "<image><name>#{img.name}</name></image>" if img.name.present?
>      provider = provider_account.provider
>      account_id = provider_account.credentials_hash['username']
> -    Aeolus::Image.import(provider.name, provider.provider_type.deltacloud_driver, image_id, account_id, xml)
> +    image = Aeolus::Image.import(provider.name, provider.provider_type.deltacloud_driver, image_id, account_id, xml)
> +    image.set_attr("environment", environment.name)
> +    image
>    end
>  end
> diff --git a/src/spec/controllers/api/images_controller_spec.rb b/src/spec/controllers/api/images_controller_spec.rb
> index ccce985..718ae9f 100644
> --- a/src/spec/controllers/api/images_controller_spec.rb
> +++ b/src/spec/controllers/api/images_controller_spec.rb
> @@ -33,6 +33,7 @@ describe Api::ImagesController do
>                      :provider => "provider")
>        @image = mock(Aeolus::Image::Warehouse::Image,
>                      :id => '5',
> +                    :environment => 'default',
>                      :os => @os,
>                      :name => 'test',
>                      :description => 'test image',
> @@ -40,15 +41,24 @@ describe Api::ImagesController do
>                      :build => @build,
>                      :provider_images => [@provider_image, @provider_image.clone]
>                      )
> -      @provider_account = mock(ProviderAccount,
> -                    :label => 'mock',
> -                    :credentials_hash => {'username' => 'foo', 'password' => 'bar'})
>        @provider_type = mock(ProviderType,
>                      :name => 'mock',
>                      :deltacloud_driver => 'mock')
>        @provider = mock(Provider,
>                      :name => 'mock',
> +                    :enabled => true,
>                      :provider_type => @provider_type)
> +      @provider_account = mock(ProviderAccount,
> +                    :label => 'mock',
> +                    :provider => @provider,
> +                    :credentials_hash => {'username' => 'foo', 'password' => 'bar'})
> +      @environment = mock(PoolFamily, :name => 'default')
> +      ProviderAccount.any_instance.stub(:pool_families).and_return([@environment])
> +      ProviderAccount.stub(:group_by_type).and_return(
> +                                 "mock" => {:type => @provider_type,
> +                                            :included => true,
> +                                            :accounts => [@provider_account, true]})
> +      PoolFamily.any_instance.stub(:provider_accounts).and_return([@provider_account])
>        Aeolus::Image::Warehouse::ImageBuild.stub(:where).and_return([@build])
>      end
>  
> @@ -82,6 +92,7 @@ describe Api::ImagesController do
>              @image_collection.each_with_index do |image, index|
>                resp['images']['image'][index]['name'].should == image.name
>                resp['images']['image'][index]['id'].should == image.id
> +              resp['images']['image'][index]['environment'].should == image.environment
>                resp['images']['image'][index]['os'].should == image.os.name
>                resp['images']['image'][index]['arch'].should == image.os.arch
>                resp['images']['image'][index]['os_version'].should == image.os.version
> @@ -105,6 +116,7 @@ describe Api::ImagesController do
>              resp = Hash.from_xml(response.body)
>              resp['images']['image']['name'].should == @image.name
>              resp['images']['image']['id'].should == @image.id
> +            resp['images']['image']['environment'].should == @image.environment
>              resp['images']['image']['os'].should == @image.os.name
>              resp['images']['image']['arch'].should == @image.os.arch
>              resp['images']['image']['os_version'].should == @image.os.version
> @@ -146,6 +158,7 @@ describe Api::ImagesController do
>              resp = Hash.from_xml(response.body)
>              resp['image']['name'].should == @image.name
>              resp['image']['id'].should == @image.id
> +            resp['image']['environment'].should == @image.environment
>              resp['image']['os'].should == @image.os.name
>              resp['image']['arch'].should == @image.os.arch
>              resp['image']['os_version'].should == @image.os.version
> @@ -231,9 +244,12 @@ describe Api::ImagesController do
>                    parent.add_child(Nokogiri::XML(tpl).root)
>                    target "mock"
>                  }
> +                environment "default"
>                }
>              end
>              Aeolus::Image::Factory::Image.stub(:new).and_return(@image)
> +            @image.stub(:targets=).and_return(@image)
> +            @image.stub(:template=).and_return(@image)
>              @image.stub(:save!)
>              Aeolus::Image::Factory::TargetImage.stub(:status).and_return(nil)
>  
> @@ -261,12 +277,14 @@ describe Api::ImagesController do
>                    child "c2"
>                  }
>                  provider_account_name "mock"
> +                environment "default"
>                }
>              end
>              Aeolus::Image::Factory::Image.stub(:new).and_return(@image)
>              ProviderAccount.stub(:find_by_label).and_return(@provider_account)
>              @provider_account.stub(:provider).and_return(@provider)
>              @image.stub(:save!)
> +            @image.stub(:set_attr)
>              # We previously stubbed this out to return nil... That's inappropriate here:
>              Aeolus::Image::Warehouse::Image.stub(:find).and_return(@image)
>              @provider_image.stub(:set_attr)
> @@ -300,12 +318,14 @@ describe Api::ImagesController do
>                    child "c4"
>                  }
>                  provider_account_name "mock2"
> +                environment "default"
>                }
>              end
>              Aeolus::Image::Factory::Image.stub(:new).and_return(@image)
>              ProviderAccount.stub(:find_by_label).and_return(@provider_account)
>              @provider_account.stub(:provider).and_return(@provider)
>              @image.stub(:save!)
> +            @image.stub(:set_attr)
>              # We previously stubbed this out to return nil... That's inappropriate here:
>              Aeolus::Image::Warehouse::Image.stub(:find).and_return(@image)
>              @provider_image.stub(:set_attr)
> diff --git a/src/spec/controllers/api/provider_images_controller_spec.rb b/src/spec/controllers/api/provider_images_controller_spec.rb
> index 162f9ee..51a2e9a 100644
> --- a/src/spec/controllers/api/provider_images_controller_spec.rb
> +++ b/src/spec/controllers/api/provider_images_controller_spec.rb
> @@ -56,7 +56,7 @@ describe Api::ProviderImagesController do
>                                                                      :build => @build)
>          @build1 = mock(Aeolus::Image::Warehouse::ImageBuild, :target_images => [@target_image, @target_image])
>          @build2 = mock(Aeolus::Image::Warehouse::ImageBuild, :target_images => [@target_image, @target_image])
> -        @image = mock(Aeolus::Image::Warehouse::Image, :image_builds => [@build1, @build2])
> +        @image = mock(Aeolus::Image::Warehouse::Image, :image_builds => [@build1, @build2], :environment => 'default')
>        end
>  
>        it "should raise bad request when no provider account is given" do
> @@ -347,6 +347,7 @@ describe Api::ProviderImagesController do
>          context "when trying to build image" do
>            before(:each) do
>              @provider_account = FactoryGirl.create :mock_provider_account
> +            @provider_account.pool_families << PoolFamily.find_by_name('default')
>              xml = Nokogiri::XML::Builder.new do |x|
>                x.provider_image {
>                  x.image_id "17"
> @@ -362,7 +363,7 @@ describe Api::ProviderImagesController do
>                                                                          :target => 'mock')
>              @build1 = mock(Aeolus::Image::Warehouse::ImageBuild, :id=>'3', :target_images => [@target_image, @target_image])
>              @build2 = mock(Aeolus::Image::Warehouse::ImageBuild, :id=>'2', :target_images => [@target_image, @target_image])
> -            @image = mock(Aeolus::Image::Warehouse::Image, :id=>'1', :image_builds => [@build1, @build2])
> +            @image = mock(Aeolus::Image::Warehouse::Image, :id=>'1', :image_builds => [@build1, @build2], :environment => 'default')
>  
>              @build1.stub(:image).and_return(@image)
>              @build2.stub(:image).and_return(@image)
> diff --git a/src/spec/controllers/deployables_controller_spec.rb b/src/spec/controllers/deployables_controller_spec.rb
> index 65583cb..72d89f4 100644
> --- a/src/spec/controllers/deployables_controller_spec.rb
> +++ b/src/spec/controllers/deployables_controller_spec.rb
> @@ -23,13 +23,14 @@ describe DeployablesController do
>      @admin_permission = FactoryGirl.create :admin_permission
>      @admin = @admin_permission.user
>      mock_warden(@admin)
> +    @catalog = FactoryGirl.create(:catalog)
>    end
>  
>    describe "#new" do
>      context "with params[:create_from_image]" do
>        before do
>          @deployable = stub_model(Deployable, :name => "test_new", :id => 1)
> -        @image = mock(Aeolus::Image::Warehouse::Image, :id => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :name => 'image1', :uuid => '3c58e0d6-d11a-4e68-8b12-233783e56d35')
> +        @image = mock(Aeolus::Image::Warehouse::Image, :id => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :name => 'image1', :uuid => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :environment => "default")
>          Aeolus::Image::Warehouse::Image.stub(:find).and_return(@image)
>        end
>  
> @@ -39,13 +40,13 @@ describe DeployablesController do
>        end
>  
>        it "returns flash[:error] when no catalog and hardware profile exists" do
> -        Catalog.stub(:list_for_user).and_return([])
> +        Catalog.stub(:list_for_user).and_return(Catalog.includes(:pool).where("1=0"))
>          get :new, :create_from_image => @image.id
>          flash[:error].should_not be_empty
>        end
>  
>        it "returns flash[:error] when no catalog exists" do
> -        Catalog.stub(:list_for_user).and_return([])
> +        Catalog.stub(:list_for_user).and_return(Catalog.includes(:pool).where("1=0"))
>          HardwareProfile.stub(:list_for_user).and_return([mock(HardwareProfile)])
>          get :new, :create_from_image => @image.id
>          flash[:error].should eql(["No catalog exists! Please create one."])
> @@ -55,7 +56,7 @@ describe DeployablesController do
>  
>    describe "#create" do
>      before(:each) do
> -      @image = mock(Aeolus::Image::Warehouse::Image, :id => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :name => 'image1', :uuid => '3c58e0d6-d11a-4e68-8b12-233783e56d35')
> +      @image = mock(Aeolus::Image::Warehouse::Image, :id => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :name => 'image1', :uuid => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :environment => "default")
>        Aeolus::Image::Warehouse::Image.stub(:find).and_return(@image)
>      end
>  
> @@ -67,8 +68,7 @@ describe DeployablesController do
>  
>      it "creates new deployable from image via POST" do
>        hw_profile = FactoryGirl.create(:front_hwp1)
> -      catalog = FactoryGirl.create(:catalog)
> -      post(:create, :create_from_image => @image.id, :deployable => {:name => @image.name}, :hardware_profile => hw_profile.id, :catalog_id => catalog.id)
> +      post(:create, :create_from_image => @image.id, :deployable => {:name => @image.name}, :hardware_profile => hw_profile.id, :catalog_id => @catalog.id)
>        response.should be_redirect
>      end
>    end
> diff --git a/src/spec/controllers/deployments_controller_spec.rb b/src/spec/controllers/deployments_controller_spec.rb
> index 278cb9d..6986ef8 100644
> --- a/src/spec/controllers/deployments_controller_spec.rb
> +++ b/src/spec/controllers/deployments_controller_spec.rb
> @@ -62,8 +62,11 @@ describe DeploymentsController do
>          Factory.create(:front_hwp1)
>          Factory.create(:front_hwp2)
>          @deployment = Factory.build(:deployment)
> +        catalog_entry = Factory.create(:catalog_entry)
> +        @deployable = catalog_entry.deployable
> +        @catalog = catalog_entry.catalog
>          Deployment.stub!(:new).and_return(@deployment)
> -        post :create
> +        post :create, :deployable_id => @deployable.id
>        end
>  
>        it { response.should be_success }
> diff --git a/src/spec/vcr/cassettes/iwhd_connection.yml b/src/spec/vcr/cassettes/iwhd_connection.yml
> index fc26a1a..0f2e6b7 100644
> --- a/src/spec/vcr/cassettes/iwhd_connection.yml
> +++ b/src/spec/vcr/cassettes/iwhd_connection.yml
> @@ -75,6 +75,7 @@
>        	<object_attr name="object_type" path="http://localhost:9090/images/53d2a281-448b-4872-b1b0-680edaad5922/object_type"/>
>        	<object_attr name="uuid" path="http://localhost:9090/images/53d2a281-448b-4872-b1b0-680edaad5922/uuid"/>
>        	<object_attr name="template" path="http://localhost:9090/images/53d2a281-448b-4872-b1b0-680edaad5922/template"/>
> +      	<object_attr name="environment" path="http://localhost:9090/images/53d2a281-448b-4872-b1b0-680edaad5922/environment"/>
>        </object>
>  
>      http_version: "1.1"
> @@ -104,6 +105,29 @@
>  - !ruby/struct:VCR::HTTPInteraction 
>    request: !ruby/struct:VCR::Request 
>      method: :get
> +    uri: http://localhost:9090/images/53d2a281-448b-4872-b1b0-680edaad5922/environment
> +    body: 
> +    headers: 
> +      accept: 
> +      - "*/*; q=0.5, application/xml"
> +      accept-encoding: 
> +      - gzip, deflate
> +      content-length: 
> +      - "0"
> +  response: !ruby/struct:VCR::Response 
> +    status: !ruby/struct:VCR::ResponseStatus 
> +      code: 200
> +      message: OK
> +    headers: 
> +      date: 
> +      - Mon, 13 Jun 2011 14:11:30 GMT
> +      content-length: 
> +      - "7"
> +    body: default
> +    http_version: "1.1"
> +- !ruby/struct:VCR::HTTPInteraction 
> +  request: !ruby/struct:VCR::Request 
> +    method: :get
>      uri: http://localhost:9090/images/53d2a281-448b-4872-b1b0-680edaad5922/latest_unpushed
>      body: 
>      headers: 
> @@ -384,6 +408,7 @@
>        	<object_attr name="target" path="http://localhost:9090/images/34c87aa0-3405-42f8-820e-309054029295/target"/>
>        	<object_attr name="target_parameters" path="http://localhost:9090/images/34c87aa0-3405-42f8-820e-309054029295/target_parameters"/>
>        	<object_attr name="template" path="http://localhost:9090/images/34c87aa0-3405-42f8-820e-309054029295/template"/>
> +      	<object_attr name="environment" path="http://localhost:9090/images/34c87aa0-3405-42f8-820e-309054029295/environment"/>
>        	<object_attr name="uuid" path="http://localhost:9090/images/34c87aa0-3405-42f8-820e-309054029295/uuid"/>
>        </object>
>  
> @@ -506,6 +531,29 @@
>  - !ruby/struct:VCR::HTTPInteraction 
>    request: !ruby/struct:VCR::Request 
>      method: :get
> +    uri: http://localhost:9090/images/34c87aa0-3405-42f8-820e-309054029295/environment
> +    body: 
> +    headers: 
> +      accept: 
> +      - "*/*; q=0.5, application/xml"
> +      accept-encoding: 
> +      - gzip, deflate
> +      content-length: 
> +      - "0"
> +  response: !ruby/struct:VCR::Response 
> +    status: !ruby/struct:VCR::ResponseStatus 
> +      code: 200
> +      message: OK
> +    headers: 
> +      date: 
> +      - Mon, 13 Jun 2011 14:11:46 GMT
> +      content-length: 
> +      - "7"
> +    body: default
> +    http_version: "1.1"
> +- !ruby/struct:VCR::HTTPInteraction 
> +  request: !ruby/struct:VCR::Request 
> +    method: :get
>      uri: http://localhost:9090/images/34c87aa0-3405-42f8-820e-309054029295/uuid
>      body: 
>      headers: 
> @@ -557,6 +605,7 @@
>        	<object_attr name="target_parameters" path="http://localhost:9090/images/ef6fd2bb-50d4-4f53-ae92-a68b01f82cf1/target_parameters"/>
>        	<object_attr name="template" path="http://localhost:9090/images/ef6fd2bb-50d4-4f53-ae92-a68b01f82cf1/template"/>
>        	<object_attr name="uuid" path="http://localhost:9090/images/ef6fd2bb-50d4-4f53-ae92-a68b01f82cf1/uuid"/>
> +      	<object_attr name="environment" path="http://localhost:9090/images/ef6fd2bb-50d4-4f53-ae92-a68b01f82cf1/environment"/>
>        </object>
>  
>      http_version: "1.1"
> @@ -678,6 +727,29 @@
>  - !ruby/struct:VCR::HTTPInteraction 
>    request: !ruby/struct:VCR::Request 
>      method: :get
> +    uri: http://localhost:9090/images/ef6fd2bb-50d4-4f53-ae92-a68b01f82cf1/environment
> +    body: 
> +    headers: 
> +      accept: 
> +      - "*/*; q=0.5, application/xml"
> +      accept-encoding: 
> +      - gzip, deflate
> +      content-length: 
> +      - "0"
> +  response: !ruby/struct:VCR::Response 
> +    status: !ruby/struct:VCR::ResponseStatus 
> +      code: 200
> +      message: OK
> +    headers: 
> +      date: 
> +      - Mon, 13 Jun 2011 14:11:48 GMT
> +      content-length: 
> +      - "7"
> +    body: default
> +    http_version: "1.1"
> +- !ruby/struct:VCR::HTTPInteraction 
> +  request: !ruby/struct:VCR::Request 
> +    method: :get
>      uri: http://localhost:9090/images/ef6fd2bb-50d4-4f53-ae92-a68b01f82cf1/uuid
>      body: 
>      headers: 
> @@ -727,6 +799,7 @@
>        	<object_attr name="latest_unpushed" path="http://localhost:9090/images/85653387-88b2-46b0-b7b2-c779d2af06c7/latest_unpushed"/>
>        	<object_attr name="object_type" path="http://localhost:9090/images/85653387-88b2-46b0-b7b2-c779d2af06c7/object_type"/>
>        	<object_attr name="uuid" path="http://localhost:9090/images/85653387-88b2-46b0-b7b2-c779d2af06c7/uuid"/>
> +      	<object_attr name="environment" path="http://localhost:9090/images/85653387-88b2-46b0-b7b2-c779d2af06c7/environment"/>
>        </object>
>  
>      http_version: "1.1"
> @@ -824,6 +897,29 @@
>      http_version: "1.1"
>  - !ruby/struct:VCR::HTTPInteraction 
>    request: !ruby/struct:VCR::Request 
> +    method: :get
> +    uri: http://localhost:9090/images/85653387-88b2-46b0-b7b2-c779d2af06c7/environment
> +    body: 
> +    headers: 
> +      accept: 
> +      - "*/*; q=0.5, application/xml"
> +      accept-encoding: 
> +      - gzip, deflate
> +      content-length: 
> +      - "0"
> +  response: !ruby/struct:VCR::Response 
> +    status: !ruby/struct:VCR::ResponseStatus 
> +      code: 200
> +      message: OK
> +    headers: 
> +      date: 
> +      - Mon, 13 Jun 2011 14:11:51 GMT
> +      content-length: 
> +      - "7"
> +    body: default
> +    http_version: "1.1"
> +- !ruby/struct:VCR::HTTPInteraction 
> +  request: !ruby/struct:VCR::Request 
>      method: :post
>      uri: http://localhost:9090/images/images_testing_uuid
>      body: op=parts
> -- 
> 1.7.6.4

Semi-ACK.

The code does work, but I have a few nits inline. I hope you'll consider
the nits, but my ACK isn't strictly contingent on them being fixed.

There is the issue we discussed in-person, that the mocking of iwhd
objects is somehow actually going through and creating real objects in
iwhd, which should definitely be fixed before pushing. Similarly, I'm
not entirely easy with what would happen if we changed an environment's
name -- the existing images would be disassociated from the environment
in Conductor if we don't go in and modify them. You had discussed doing
this in an after_update hook if the :name parameter were changed. (I
strongly suspect you're already familiar with it, but just in case,
ActiveRecord::Dirty's obj.changes is probably handy here[1].)

If you're able to fix the issues above and at least have a look at my
nits, feel free to go ahead and push.

-- Matt

[1] http://ar.rubyonrails.org/classes/ActiveRecord/Dirty.html



More information about the aeolus-devel mailing list