On 02/06/10 19:55 +0100, [email protected] wrote:
Some comments inline:
>+get '/api/firewall_rules/new' do
>+ respond_to do |format|
>+ format.html { haml :"firewall_rules/new" }
>+ end
>+end
>+
>+
>+def delete_firewall_rule(service_id)
>+ driver.send(delete_firewall_rule, credentials, {:id => params[:id]})
>+end
>+
> # Rabbit DSL
>+collection :firewall_rules do
>+
>+ description "Allow user to expose a public IP address/port and map these
>onto an internal IP address/port. eg expose ssh access [port 22, tcp]."
Please use:
description <<END
Allow user to expose a public IP address/port and map these onto an
internal IP address/port. eg expose ssh access [port 22, tcp]
END
here to keep code 'clean' ;-) I recently updated all description here to
use this syntax.
>+ operation :index do
>+ description 'Show all available network services'
>+ control { filter_all(:firewall_rules) }
>+ end
>+
>+ operation :show do
>+ description 'Show a firewall rule identified by its id'
>+ param :id, :string, :required
>+ control { show(:firewall_rule) }
>+ end
>+
>+ operation :create do
>+ description 'Create a new firewall rule'
>+ param :public_address, :string, :required
>+ param :public_port, :string, :required
>+ param :private_address, :string, :required
>+ param :private_port, :string, :required
>+ param :service_protocol, :string, :required
>+ control do
>+ driver.create_firewall_rule(credentials, params )
>+ filter_all(:firewall_rules)
>+ end
>+ end
>+
>+ operation :destroy, :method => :post do
Actually, there is no need to specify a 'method'. Method for ':destroy'
action is already defined in rabbit.rb (DELETE).
>+ description 'destroy given firewall rule'
>+ param :id, :string, :required
>+ control { delete_firewall_rule(:id) }
>+ end
>+end
>
> collection :realms do
> description "Within a cloud provider a realm represents a boundary
> containing resources. The exact definition of a realm is left to the cloud
> provider. In some cases, a realm may represent different datacenters,
> different continents, or different pools of resources within a single
> datacenter. A cloud provider may insist that resources must all exist within
> a single realm in order to cooperate. For instance, storage volumes may only
> be allowed to be mounted to instances within the same realm."
>@@ -186,6 +241,22 @@ collection :instance_states do
> end
> end
>
>+# Special instance get operations that we only allow for HTML
>+get "/api/instances/:id/:action" do
>+ meth = :"#{params[:action]}_instance"
>+ not_found unless driver.respond_to?(meth)
>+ respond_to do |format|
>+ format.html do
>+ driver.send(meth, credentials, params[:id])
>+ if ( (params[:action] == 'destroy') or (params[:action] == 'stop') )
>+ redirect instances_url
>+ else
>+ redirect instance_url(params[:id])
>+ end
>+ end
>+ end
>+end
>+
Also this method is no longer needed. Please pull/rebase your patch.
Actions which require POST/DELETE HTTP methods are now called using
Javascript from HTML UI.
> get "/api/instances/new" do
> @instance = Instance.new( { :id=>params[:id], :image_id=>params[:image_id]
> } )
> @image = driver.image( credentials, :id => params[:image_id] )
>@@ -197,14 +268,15 @@ get "/api/instances/new" do
> end
>
> def instance_action(name)
>- @instance = driver.send(:"#{name}_instance", credentials, params["id"])
>-
>- return redirect(instances_url) if name.eql?(:destroy) or
>@instance.class!=Instance
>-
>- respond_to do |format|
>- format.html { haml :"instances/show" }
>- format.xml { haml :"instances/show" }
>- format.json {convert_to_json(:instance, @instance) }
>+ @instance = driver.send(:"#{name}_instance", credentials, params[:id])
>+ if name==:destroy
>+ redirect instances_url
>+ else
>+ respond_to do |format|
>+ format.html { haml :"instances/show" }
>+ format.xml { haml :"instances/show" }
>+ format.json {convert_to_json(:instance, @instance) }
>+ end
> end
> end
>
>diff --git a/server/views/firewall_rules/index.html.haml
>b/server/views/firewall_rules/index.html.haml
>new file mode 100644
>index 0000000..47990b0
>--- /dev/null
>+++ b/server/views/firewall_rules/index.html.haml
>@@ -0,0 +1,37 @@
>+%h1
>+ Firewall Rules
>+%h3
>+ = link_to("Create new firewall rule", firewall_rules_url() + "/new")
>+%table.display
>+ %thead
>+ %tr
>+ %th
>+ ID
>+ %th
>+ Public Address
>+ %th
>+ Public Port
>+ %th
>+ Private Address
>+ %th
>+ Private Port
>+ %th
>+ Protocol
>+ %tbody
>+ - if @firewall_rules
>+ - for rule in @firewall_rules
>+ %tr
>+ %td
>+ = link_to rule.id, firewall_rules_url()+"/#{rule.id}"
>+ %td
>+ = rule.public_address
>+ %td
>+ = rule.public_port
>+ %td
>+ = rule.private_address
>+ %td
>+ = rule.private_port
>+ %td
>+ = rule.protocol
>+ %td
>+ = link_to("destroy", firewall_rules_url()+"/#{rule.id}/destroy",
>{:method => :post})
There should be a 'destroy_firewall_rule(rule.id)' helper (once you declare
operation in firewall_rules collection, you will get helper for free ;-))
>\ No newline at end of file
>diff --git a/server/views/firewall_rules/new.html.haml
>b/server/views/firewall_rules/new.html.haml
>new file mode 100644
>index 0000000..9c6f0a0
>--- /dev/null
>+++ b/server/views/firewall_rules/new.html.haml
>@@ -0,0 +1,22 @@
>+%h1 New Firewall Rule
>+
>+%form{ :action => firewall_rules_url, :method => :post }
>+ %p
>+ %label
>+ Public Address:
>+ %input{ :name => 'public_address', :size => 16 }
Hmm I spot :size here and it will be a good idea to move size validation to
Rabbit. Right now we have just 'type' and 'required/optional' validation.
/me taking a note ;-)
>+ %label
>+ Public Port:
>+ %input{ :name => 'public_port', :size => 6}
>+ %label
>+ Protocol:
>+ %input{ :name => 'service_protocol', :size => 5}
>+ %p
>+ %label
>+ Private Address:
>+ %input{ :name => 'private_address', :size => 16 }
>+ %label
>+ Private Port:
>+ %input{ :name => 'private_port', :size => 6}
>+ %p
>+ %input{ :type => :submit, :name => 'commit', :value => "create" }
>\ No newline at end of file
>diff --git a/server/views/firewall_rules/show.html.haml
>b/server/views/firewall_rules/show.html.haml
>new file mode 100644
>index 0000000..8ec81c2
>--- /dev/null
>+++ b/server/views/firewall_rules/show.html.haml
>@@ -0,0 +1,21 @@
>+%h1 Firewall Rule
>+%h2
>+ = @firewall_rule.id
>+
>+%dl
>+ %di
>+ %dt Public IP
>+ %dd
>+ = @firewall_rule.public_address
>+ %dt Public Port
>+ %dd
>+ = @firewall_rule.public_port
>+ %dt Private IP
>+ %dd
>+ = @firewall_rule.private_address
>+ %dt Private Port
>+ %dd
>+ = @firewall_rule.private_port
>+ %dt Protocol
>+ %dd
>+ = @firewall_rule.protocol
>\ No newline at end of file
Actually this patch looks great and I looking forward for EC2 version.
Good job!
-- Michal
--
--------------------------------------------------------
Michal Fojtik, [email protected], +420 532 294 4307
Ruby / Ruby On Rails Developer
Deltacloud API: http://deltacloud.org
--------------------------------------------------------
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
