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. 
 
    
  
  
    
  
 

   
    
    
    




    
    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. 
 
    
  
  
    
  
 

   
    
    
    




    
    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. 




  

 









     

    

   

  

  

   
   Loading...