Skip to content
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

Add method to get metrics from PowerSupply #389

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

smiller248
Copy link
Contributor

It appears that there is both a PowerSupply and a PowerSupplyUnit type, which both represent the same Redfish "PowerSupply" resource.

Unfortunately, PowerSupply and PowerSuppyUnit each have some features that the other does not. For my current use case, I need some fields that only exist on PowerSupply but also require the Metrics() method, which is only available on PowerSupplyUnit.

I have opted for that path of least resistance here: just adding a link to PowerSupplyUnitMetrics from PowerSupply (since it's the same underlying Redfish resource schemas, everything parses correctly). Note: there is no PowerSupplyMetrics.

Please let me know if:

  1. There is a good reason that PowerSupply and PowerSupplyUnit are distinct, which am oblivious of
  2. You would prefer a more comprehensive fix (probably with breaking changes) rather than the path-of-least-resistance option

Copy link
Owner

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I didn't review closely enough when PowerSupplyUnit was added. That is indeed just a PowerSupply.

We should just collapse its content into PowerSupply and get rid of it. That may break some folks, but it makes sense to do. And since Gofish is still a v0.x release, I don't feel too badly about it.

This is good for now. If you'd like to follow up with more, that would be awesome. Otherwise I'll put it on my list to try to get to some day.

Thanks!

@stmcginnis stmcginnis merged commit cca588b into stmcginnis:main Dec 5, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants