+1, although the external node tool looks up nodes by names, so it will still break for certain names. It might be worth making a new URL used explicitly for node classification, as realistically, it makes more sense to include all information about a node when serializing it to YAML, rather than only the information used for node classification.
On Thu, Sep 30, 2010 at 4:24 PM, Jacob Helwig <[email protected]> wrote: > Routing to the nodes using their names prevented some "legal" (to Puppet) > node names from being used ("new", for example). Change the default > routing to use the node.id to get around this problem. Also, fall back to > looking up the node by name for backwards compatibility. > > Signed-off-by: Jacob Helwig <[email protected]> > --- > > This is also available at: > git://github.com/jhelwig/puppet-dashboard.gitbug-4541-node-routing-using-id > > app/controllers/nodes_controller.rb | 8 +++++++- > app/models/node.rb | 4 ---- > spec/controllers/nodes_controller_spec.rb | 17 +++++++++++++++-- > spec/models/node_spec.rb | 8 ++++++-- > spec/views/nodes/edit.html.haml_spec.rb | 2 +- > 5 files changed, 29 insertions(+), 10 deletions(-) > > diff --git a/app/controllers/nodes_controller.rb > b/app/controllers/nodes_controller.rb > index 8301858..d881fba 100644 > --- a/app/controllers/nodes_controller.rb > +++ b/app/controllers/nodes_controller.rb > @@ -53,7 +53,13 @@ class NodesController < InheritedResources::Base > protected > > def resource > - get_resource_ivar || > set_resource_ivar(end_of_association_chain.find_by_name!(params[:id])) > + node = get_resource_ivar > + return node if node > + > + node ||= end_of_association_chain.find(params[:id]) rescue nil > + node ||= end_of_association_chain.find_by_name!(params[:id]) > + > + set_resource_ivar(node) > end > > # Render the index using the +scope_name+ (e.g. :successful for > Node.successful). > diff --git a/app/models/node.rb b/app/models/node.rb > index ddfbb92..80b1c34 100644 > --- a/app/models/node.rb > +++ b/app/models/node.rb > @@ -84,10 +84,6 @@ class Node < ActiveRecord::Base > no_longer_reporting.count > end > > - def to_param > - name.to_s > - end > - > def available_node_classes > @available_node_classes ||= NodeClass.all(:order => :name) - > node_classes - inherited_classes > end > diff --git a/spec/controllers/nodes_controller_spec.rb > b/spec/controllers/nodes_controller_spec.rb > index 4837695..f7c0655 100644 > --- a/spec/controllers/nodes_controller_spec.rb > +++ b/spec/controllers/nodes_controller_spec.rb > @@ -114,7 +114,7 @@ describe NodesController do > end > > def do_get > - get :edit, :id => @node.name > + get :edit, :id => @node.id > end > > it 'should make the requested node available to the view' do > @@ -126,12 +126,18 @@ describe NodesController do > do_get > response.should render_template('edit') > end > + > + it 'should work when given a node name' do > + get :edit, :id => @node.name > + > + assigns[:node].should == @node > + end > end > > describe '#update' do > before :each do > @node = Node.generate! > - @params = { :id => @node.name, :node => @node.attributes } > + @params = { :id => @node.id, :node => @node.attributes } > end > > def do_put > @@ -143,6 +149,13 @@ describe NodesController do > lambda { do_put }.should raise_error(ActiveRecord::RecordNotFound) > end > > + it 'should work when given a node name' do > + @params.merge!({:id => @node.name}) > + > + do_put > + assigns[:node].should == @node > + end > + > describe 'when a valid node id is given' do > > describe 'and the data provided would make the node invalid' do > diff --git a/spec/models/node_spec.rb b/spec/models/node_spec.rb > index 27ce369..58b1a59 100644 > --- a/spec/models/node_spec.rb > +++ b/spec/models/node_spec.rb > @@ -143,7 +143,9 @@ describe Node do > end > > describe 'when the node has classes' do > - before { @node.node_classes << @node_classes.first } > + before :each do > + @node.node_classes << @node_classes.first > + end > > it "should not include the node's classes" do > @node.available_node_classes.should_not > include(@node_classes.first) > @@ -163,7 +165,9 @@ describe Node do > end > > describe 'when the node has groups' do > - before { @node.node_groups << @node_groups.first } > + before :each do > + @node.node_groups << @node_groups.first > + end > > it "should not include the node's groups" do > @node.available_node_groups.should_not include(@node_groups.first) > diff --git a/spec/views/nodes/edit.html.haml_spec.rb > b/spec/views/nodes/edit.html.haml_spec.rb > index 9177996..eff2c1a 100644 > --- a/spec/views/nodes/edit.html.haml_spec.rb > +++ b/spec/views/nodes/edit.html.haml_spec.rb > @@ -3,7 +3,7 @@ require File.expand_path(File.join(File.dirname(__FILE__), > *%w[.. .. spec_helper > describe '/nodes/edit' do > before :each do > assigns[:node] = @node = Node.generate! > - params[:id] = @node.name > + params[:id] = @node.id > end > > def do_render > -- > 1.7.3 > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
