make post-review able to handle customized reviewboard URL namespaces

 






 

 


  Review Request #360
  —
  Created April 16, 2008 and submitted

 


















 

  

   Information

  





 
 

  edgimar
 









 
 

  Review Board SVN (deprecated)
 









 
 

  
 









 
 

  457
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This patch allows the post-review tool to work in cases where the reviewboard URL contains a prefix (e.g. http://my.server.com/reviewboard/...). 

I also cleaned up / added some documentation to the script.

 








  


 
 

  
Diffs were successfully posted to a 'prefixed' server path with it, as well as to this server.

 







 









 





  








 









 






  

 








 














 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Thanks for doing this. Only a couple small comments.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Can you add "457" to the bugs field on this review request so we remember to close it when this gets committed?

Thanks.
      
 
      

      2. 


 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       /trunk/reviewboard/contrib/tools/post-review
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    I'm not sure we want to enforce a trailing slash. Repository paths are not necessarily always going to be directories.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      This may be true.  The issue is that the urljoin function will strip off of its left argument anything to the right of the last '/'.  Perhaps urljoin is the wrong function to use?  Or the user should be warned in a comment near the REVIEWBOARD_URL assignment that the trailing slash is necessary.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      I think we should probably move this code to the repository checking. We can normalize the URL and make sure the trailing slashes match. A quick way of doing this is to look up both variants when trying to find the database entry.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Do you mean to the scan_for_server method?  If so, I guess it would mean having to add it to every SCMClient subclass (or a method to SCMClient which was called from each subclass)?  I'll leave it to someone else to implement it.  Feel free to update the diff if you come up with something you like.
      
 
      

      4. 



    4. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry, I wrote this just before leaving for work and didn't explain it well.

I was thinking more on the Review Board side of things. It could be more intelligent with its server lookups so that we only have to do this in one place.
      
 
      

      5. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       /trunk/reviewboard/contrib/tools/post-review
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    We could still combine these into one path.

review_url = urljoin(server.url, "r/%s" % review_request['id'])
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I have no objections to doing that.
      
 
      

      2. 


 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5.