Fix various aspects of dispatching webhook with auth credentials

 






 

 


  Review Request #11171
  —
  Created Sept. 14, 2020 and discarded 

 


















 

  

   Information

  





 
 

  yxejamir
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Fix error dispatching webhook with auth credentials


The error was:


AttributeError: 'SplitResult' object has no attribute 'params'



The 5-tuple used for rebuilding the URL was missing the fragment element instead.




Log webhook server responses on error


If the exception from urlopen represents an HTTP response, then the included

information may be useful to the site admin in troubleshooting.




Always send Authorization header on authenticated webhooks


The problem with HTTPBasicAuthHandler is that it will first attempt to make an

unauthenticated request, and will retry with authentication only if a 401

response was seen. However, some servers respond with 403 if the right

credentials are not supplied right away.

 








  


 
 

  
This is a forward-ported version of my patches on top of 3.0.18, which is powering our production site.


Note: It is best to review the changes one commit at a time. The patch series is available publicly on Github.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
From the description, it looks like there's multiple independent changes all being introduced. Can you split this into a review …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
E501 line too long (81 > 79 characters)

      		
      
       reviewbotreviewbot
      
      
       
      
     




     

      
       
       
Can you reorder the new import to be in alphabetical order? Helps us keep our imports organized.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Now that the list of imports here is short, will it fit on one line?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
This can be placed earlier up, in alphabetical order.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
We prefer a blank line between statements and new blocks (helps keep blocks from running together, avoiding issues with, say, …

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 



















 




 

  


   

   

    
 
Checks run (1 failed, 1 succeeded)


    
   


  

  

  





 

 flake8
 failed.
 





 

 JSHint
 passed.
 













 
flake8

 


 










  

 





 














 



 

  

   





 YX





   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 







  2. 
 
    
  
  
    
  
   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    From the description, it looks like there's multiple independent changes all being introduced. Can you split this into a review request per change, so we can more easily get these in and keep them separate?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      No problem. I will have to figure out later, what the best way is to submit a patch series for review, like you would with git send-email <rev-list options>.
      
 
      

      2. 


 
    


    

 
    

    3. 















  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/webhooks.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Can you reorder the new import to be in alphabetical order? Helps us keep our imports organized.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/webhooks.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Now that the list of imports here is short, will it fit on one line?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      As a side note: for new projects I would consider introducing Black into the development workflow.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/webhooks.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This can be placed earlier up, in alphabetical order.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/notifications/webhooks.py
      


       (Diff revision 2)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    We prefer a blank line between statements and new blocks (helps keep blocks from running together, avoiding issues with, say, two if blocks appearing as part of the same set of conditionals).
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  7. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 




  

 
















 



 

  

   





 YX





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Discarded

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...