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.

Sign up for GitHub

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

Jump to bottom

✨ Add a list of alternate disk names to HardwareDetails #1269

Merged
merged 2 commits into from
Sep 20, 2023
Merged

✨ Add a list of alternate disk names to HardwareDetails #1269

merged 2 commits into from
Sep 20, 2023

Conversation

zaneb
Copy link
Member

@zaneb zaneb commented May 3, 2023

This is a follow up (and depends on) #1264.

Give users a way of seeing all of the alternate names for the disk that
we have discovered, even if they are not stable across reboots.

@zaneb zaneb requested review from dtantsur and s3rj1k May 3, 2023 21:26
@metal3-io-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2023
@zaneb zaneb mentioned this pull request May 3, 2023
Merged
s3rj1k
s3rj1k reviewed May 3, 2023
View reviewed changes
apis/metal3.io/v1alpha1/baremetalhost_types.go
// A list of alternate Linux device names of the disk, e.g. "/dev/sda".
// Note that this list is not exhaustive, and names may not be stable
// across reboots.
AlternateNames []string `json:"alternateNames,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

To clarify this will be a list of known device names from Inspection excluding the one in Name field?

Also maybe we should sort | uniq this list somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the proposed implementation it includes the name in the name field, and entries are unique. Also, the true kernel device (e.g. /dev/sda) always appears first.

@zaneb zaneb marked this pull request as ready for review May 9, 2023 09:39
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 9, 2023
zaneb added 2 commits May 10, 2023 09:15
@zaneb
Improve documentation of Storage 'Name' in HardwareDetails
15d9c73
@zaneb
Add a list of alternate disk names to HardwareDetails
505781a 
Give users a way of seeing all of the alternate names for the disk that
we have discovered, even if they are not stable across reboots.
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2023
@honza
Copy link
Member

honza commented Jun 21, 2023

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: honza

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2023
@hardys
Copy link
Member

hardys commented Sep 19, 2023

/test-ubuntu-integration-main
/test-centos-e2e-integration-main

@hardys
Copy link
Member

hardys commented Sep 20, 2023

/lgtm

@metal3-io-bot
Copy link
Contributor

@hardys: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@hardys
Copy link
Member

hardys commented Sep 20, 2023

@dtantsur or @s3rj1k perhaps you can take a look since you previously reviewed this and I'm no longer an approver :)

@tuminoid
Copy link
Member

Please add PR type (:seedling: etc) to the PR title.

@hardys
Copy link
Member

hardys commented Sep 20, 2023

/retitle ✨ Add a list of alternate disk names to HardwareDetails

@metal3-io-bot metal3-io-bot changed the title Add a list of alternate disk names to HardwareDetails ✨ Add a list of alternate disk names to HardwareDetails Sep 20, 2023
@s3rj1k
Copy link
Member

s3rj1k commented Sep 20, 2023

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2023
@metal3-io-bot metal3-io-bot merged commit bdfa1bd into metal3-io:main Sep 20, 2023
10 checks passed
@zaneb
Copy link
Member Author

zaneb commented Sep 21, 2023

@hardys looks like you need to submit a review to move yourself out of OWNERS purgatory 😄 (welcome back!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtantsur dtantsur dtantsur left review comments

@s3rj1k s3rj1k s3rj1k left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zaneb @metal3-io-bot @honza @hardys @tuminoid @s3rj1k @dtantsur