-
Couldn't load subscription status.
- Fork 193
[psud] Refactor PSU object retrieval to improve exception handling #659
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 16 commits
cf713d9
6063a6e
188652c
455572d
048fc91
bf4dede
cf7161e
5bdf678
2fcc059
9a11b05
3995d5f
98bee9b
a7e21b5
8a52b30
508d208
0a37102
23efbcf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -90,31 +90,76 @@ exit_code = 0 | |||||||||||||||||||||||||
| # temporary wrappers that are compliable with both new platform api and old-style plugin mode | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| def _wrapper_get_num_psus(): | ||||||||||||||||||||||||||
| def _wrapper_get_num_psus(logger): | ||||||||||||||||||||||||||
| if platform_chassis is not None: | ||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||
| return platform_chassis.get_num_psus() | ||||||||||||||||||||||||||
| except NotImplementedError: | ||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||
| return platform_psuutil.get_num_psus() | ||||||||||||||||||||||||||
| if platform_psuutil is not None: | ||||||||||||||||||||||||||
| return platform_psuutil.get_num_psus() | ||||||||||||||||||||||||||
| if logger: | ||||||||||||||||||||||||||
| logger.log_warning("PSU provider unavailable; defaulting to 0 PSUs") | ||||||||||||||||||||||||||
| return 0 | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| return 0 | |
| raise RuntimeError("Unable to determine number of PSUs: neither platform_chassis nor platform_psuutil is available") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a warning log something like, if we have to return 0
if logger:
logger.log_warning("No PSU provider available; assuming 0 PSUs")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Copilot
AI
Oct 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring is missing the logger parameter description. Add ':param logger: Logger instance for error/warning messages' to document the logger parameter.
| Get PSU object from platform chassis | |
| Get PSU object from platform chassis | |
| :param logger: Logger instance for error/warning messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LinJin23 Pls fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring should clarify that logger can be None and that when None is passed, errors/warnings won't be logged. This is an important behavior detail for callers of this function.
| Get PSU object from platform chassis | |
| :param logger: Logger instance for error/warning messages | |
| :param psu_index: PSU index (1-based) | |
| :return: PSU object if available, None otherwise | |
| Get PSU object from platform chassis. | |
| :param logger: Logger instance for error/warning messages. Can be None; if None is passed, errors and warnings will not be logged. | |
| :param psu_index: PSU index (1-based) | |
| :return: PSU object if available, None otherwise | |
| Note: | |
| If logger is None, errors and warnings encountered during execution will not be logged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be less than a warning imo, maybe a notice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vvolam, you mentioned in an earlier comment that this should use a warning level, so I’m a bit unsure which one to use here. Could you please help confirm which level is appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes NotImplemented exception could be notice as we are logging warning for failed case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice is not available, so you can leave this as warning for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, got it, I’ll keep it as warning for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to return False for other exceptions in this conditional statement too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, add another condition as
"except Exception as e:
return False
"
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment explaining the IndexError exception handling was removed. This comment provided valuable context about why the exception is caught and should be preserved: 'some functionality is expectent on returning an expected key even if the psu object itself does not exist'.
| pass | |
| pass | |
| # some functionality is expectent on returning an expected key even if the psu object itself does not exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is logger being passed in here? its not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out. I’ve removed it.