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 drives and volumes to server.status.storages #163

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

stefanhipfel
Copy link
Contributor

@stefanhipfel stefanhipfel commented Nov 4, 2024

In redfish storage controllers are made up of volumes and drives.

This change reflects this structure and shows all drives and volumes of each storage available on this server

@stefanhipfel stefanhipfel requested a review from defo89 November 4, 2024 15:36
@github-actions github-actions bot added api-change documentation Improvements or additions to documentation size/L labels Nov 4, 2024
Copy link
Member

@afritzler afritzler left a comment

Choose a reason for hiding this comment

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

Thanks a lot @stefanhipfel. Please find my comments below.

bmc/bmc.go Outdated
Entity
State common.State `json:"state,omitempty"`
// Description provides a description of this resource.
Description string `json:"description,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Do we really wan to have a description field in the status? Where is this information coming from and what should the controller do with this information? Or is it just for the information of the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just in case vendors put some useful information in there ;)

can be removed

Copy link
Member

Choose a reason for hiding this comment

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

Lets keep this out for now. We can always add fields. Removing is always a pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is anyway not in the server crd api, just our internal bmc storage structure.

@afritzler afritzler changed the title adds drives and volumes to Status.Storages Add drives and volumes to server.status.storages Nov 5, 2024
@afritzler
Copy link
Member

Can you please add some PR comments describing the proposed status change?

@afritzler afritzler merged commit 2026e3d into main Nov 5, 2024
8 checks passed
@afritzler afritzler deleted the storage_drives_volumes branch November 5, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change breaking documentation Improvements or additions to documentation size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants