fix: update BMC credentials code with Bmc class. #1558
Conversation
python/understack-workflows/understack_workflows/bmc_credentials.py
Outdated
Show resolved
Hide resolved
python/understack-workflows/understack_workflows/bmc_credentials.py
Outdated
Show resolved
Hide resolved
python/understack-workflows/understack_workflows/bmc_credentials.py
Outdated
Show resolved
Hide resolved
RSabounds
force-pushed
the
PUC-1410
branch
2 times, most recently
from
a26a8b9 to
93b32ca
Compare
|
pytest and precommit successfully completed. Typos Spell Checker / spellcheck not related to this PR. issue exists in MAIN branch. |
|
|
||
| def get_base_path(self): | ||
| """Get Base Path.""" | ||
| _result = "/redfish/v1/" |
There was a problem hiding this comment.
This has changed from "/redfish/v1" - it now has a trailing slash. Not sure if that matters to the BMC.
There was a problem hiding this comment.
usually doesn't matter, some systems like HP have more issue if there is or isn't a trailing slash. This was just how it was tested as functional. Can change if preference.
There was a problem hiding this comment.
applying and testing with HP in case of issue.
| if token: | ||
| a = self.redfish_request(path=account_url, token=token) | ||
| else: | ||
| a = self.redfish_request(path=account_url) |
There was a problem hiding this comment.
It looks like redfish_request would treat the parameter token=None the same as if the parameter is missing, and the type signature agrees with this:
| if token: | |
| a = self.redfish_request(path=account_url, token=token) | |
| else: | |
| a = self.redfish_request(path=account_url) | |
| a = self.redfish_request(path=account_url, token=token) |
There was a problem hiding this comment.
wrote this way originally. I added the if token and else because the precommit was flagging it for cases of the token being None.
I can remove this and rerun the test again.
| logger.debug("found account: %s", a) | ||
| matched_account = a | ||
| break | ||
| if not matched_account: |
There was a problem hiding this comment.
This looks like it should be after (outside) the loop. Copy and paste error?
There was a problem hiding this comment.
yeah, recopied code after rebase faulted. looks like the formatting got fluxed. fixed.
| else: | ||
| self.redfish_request(method="PATCH", path=account_uri, payload=_payload) | ||
|
|
||
| def get_session(self, password: str) -> tuple[str | None, str | None]: |
There was a problem hiding this comment.
| def get_session(self, password: str) -> tuple[str | None, str | None]: | |
| def get_session(self, password: str) -> tuple[str, str] | tuple[None, None]: |
| payload: dict | None = None, | ||
| verify: bool = False, | ||
| timeout: int = 30, | ||
| ) -> tuple[str | None, str | None]: |
There was a problem hiding this comment.
| ) -> tuple[str | None, str | None]: | |
| ) -> tuple[str, str] | tuple[None, None]: |
| if token: | ||
| self.redfish_request(method="DELETE", path=session, token=token) | ||
| else: | ||
| self.redfish_request(method="DELETE", path=session) |
There was a problem hiding this comment.
Same comment as above, can refactor to eliminate the condition
| def get_user_accounts(self, token: str | None = None) -> list[dict]: | ||
| """A vendor agnostic approach to crawling the API for BMC accounts.""" | ||
| try: | ||
| # get account service | ||
| r = ( | ||
| self.redfish_request(path=self.base_path, token=token) | ||
| if token | ||
| else self.redfish_request(path=self.base_path) | ||
| ) | ||
| account_service_uri = r["AccountService"]["@odata.id"] | ||
| logger.debug("account_service_url: %s", account_service_uri) | ||
|
|
||
| # get account collection uri | ||
| r = ( | ||
| self.redfish_request(path=account_service_uri, token=token) | ||
| if token | ||
| else self.redfish_request(path=account_service_uri) | ||
| ) | ||
| accounts_uri = r["Accounts"]["@odata.id"] | ||
| logger.debug("accounts_url: %s", accounts_uri) | ||
|
|
||
| # get accounts | ||
| r = ( | ||
| self.redfish_request(path=accounts_uri, token=token) | ||
| if token | ||
| else self.redfish_request(path=accounts_uri) | ||
| ) | ||
| accounts = r["Members"] | ||
| logger.debug("accounts: %s", accounts) | ||
|
|
||
| return accounts | ||
| except AccountServiceException: | ||
| logger.exception("Can't fetch accounts from Redfish account service.") | ||
| raise |
There was a problem hiding this comment.
This is quite verbose, it feels like this would behave the same:
| def get_user_accounts(self, token: str | None = None) -> list[dict]: | |
| """A vendor agnostic approach to crawling the API for BMC accounts.""" | |
| try: | |
| # get account service | |
| r = ( | |
| self.redfish_request(path=self.base_path, token=token) | |
| if token | |
| else self.redfish_request(path=self.base_path) | |
| ) | |
| account_service_uri = r["AccountService"]["@odata.id"] | |
| logger.debug("account_service_url: %s", account_service_uri) | |
| # get account collection uri | |
| r = ( | |
| self.redfish_request(path=account_service_uri, token=token) | |
| if token | |
| else self.redfish_request(path=account_service_uri) | |
| ) | |
| accounts_uri = r["Accounts"]["@odata.id"] | |
| logger.debug("accounts_url: %s", accounts_uri) | |
| # get accounts | |
| r = ( | |
| self.redfish_request(path=accounts_uri, token=token) | |
| if token | |
| else self.redfish_request(path=accounts_uri) | |
| ) | |
| accounts = r["Members"] | |
| logger.debug("accounts: %s", accounts) | |
| return accounts | |
| except AccountServiceException: | |
| logger.exception("Can't fetch accounts from Redfish account service.") | |
| raise | |
| def get_user_accounts(self, token: str | None = None) -> list[dict]: | |
| """Get the list of User Accounts on this BMC.""" | |
| path = self.base_path | |
| path = self.redfish_request(path, token=token)["AccountService"]["@odata.id"] | |
| path = self.redfish_request(path, token=token)["Accounts"]["@odata.id"] | |
| return self.redfish_request(path, token=token)["Members"] |
There was a problem hiding this comment.
original code, just shifted.
Agreed. did not like the longer code, so appreciate the shift. added.
|
It was hard to see what had actually changed in this PR because the changes are hidden by a massive refactoring. I would suggest making the commit history look like:
|
|
rebase on main should fix CI |
Add appropriate testing.
Update hard-coded and separate Bmc communication code for bmc_credentials to use Bmc objects, handle different Dell and HP bmc types.