Feature add uname pw config param #18
Conversation
…and password parameters
…d password parameters
There was a problem hiding this comment.
Hi @SColson82 great work adding the authentication capability to sdx-lib! Very nice to see how this is shaping up and also a very important feature. Just left two comments:
- Check my comment about passing none values directly to requests lib, which already does some validation on that direction so your code can be simplified
- Please check this line here: I believe that request call should also be updated to include the authentication parameters, no?
Line 1002 in 06d6ed4
thanks and congratulations again!
…missed addition of auth into response
There was a problem hiding this comment.
Hi @SColson82 excellent work!! Just left another comment, which if accepted also applies for all other usage cases!
| @@ -517,7 +547,9 @@ def create_l2vpn(self) -> SDXResponse: | |||
| return SDXResponse(response_json) | |||
|
|
|||
| try: | |||
| response = requests.post(url, json=payload, timeout=120) | |||
| # Include authentication if username and password are provided. | |||
| auth = (self._http_username, self._http_password) | |||
There was a problem hiding this comment.
Hi @SColson82 since you are using the attribute (self._http_username and self._http_password) instead of the property (self.http_password), I wonder if the property is really necessary. Maybe you change all the usage of those attributes to refer to the property. I mean:
| auth = (self._http_username, self._http_password) | |
| auth = (self.http_username, self.http_password) |
There was a problem hiding this comment.
Hi @SColson82 @italovalcy, excellent initiative; please take a look here:
https://docs.google.com/document/d/1oRfxWk1q560OY-Seo5_ZVCuYWNc9zwmC4LseEHrMVuI/edit?tab=t.0
Feel free to adapt the code and documentation based on the specifications.
Page 6: FABRIC, OpenID Connect (OIDC), and SDX-Lib
There was a problem hiding this comment.
Hi @lmarinve! The http_username and http_password properties are meant to be a stop gap measure to address an issue that Italo and Jeronimo experienced on the SC floor. I have created issue #19 - Add more sophisticated way of authenticating - to address the more sophisticated way of authenticating once that is in place.
There was a problem hiding this comment.
@SColson82 can you please double check the points I mentioned before about the usage of private attributes versus getter/property function?
There was a problem hiding this comment.
@SColson82 left a comment regarding the usage of private attributes versus property/getter function
| @@ -517,7 +547,9 @@ def create_l2vpn(self) -> SDXResponse: | |||
| return SDXResponse(response_json) | |||
|
|
|||
| try: | |||
| response = requests.post(url, json=payload, timeout=120) | |||
| # Include authentication if username and password are provided. | |||
| auth = (self._http_username, self._http_password) | |||
There was a problem hiding this comment.
@SColson82 can you please double check the points I mentioned before about the usage of private attributes versus getter/property function?
Add optional http_username and http_password parameters to sdx_client constructor class.
Add getters and setters for both.
Adjust post, patch, get, delete methods to account for these.
Adjust post, patch, get, delete unit tests to account for these.
Add unit tests for new parameters.
Add test variables for each to test_config file.