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

Adding ipmi port to BaseboardManagement type #4

Merged
merged 2 commits into from
May 17, 2022

Conversation

pokearu
Copy link
Contributor

@pokearu pokearu commented May 17, 2022

Signed-off-by: Aravind Ramalingam ramaliar@amazon.com

Description

Added Port to BaseboardManagement.Spec.Connection. Port represents the IPMI port of the BMC.

Why is this needed

Removes port hardcoding to default IPMI UDP port 623

Signed-off-by: Aravind Ramalingam <ramaliar@amazon.com>
Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

Thanks! Lets make this an int

@@ -64,6 +64,10 @@ type Connection struct {
// +kubebuilder:validation:MinLength=1
Host string `json:"host"`

// Port is the IPMI port of the BaseboardManagement.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this could be HTTP/Redfish, can you make this wording generic? Can you state what a default value is, if any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think default would be 623. I can set a kubebuilder:default

@@ -64,6 +64,10 @@ type Connection struct {
// +kubebuilder:validation:MinLength=1
Host string `json:"host"`

// Port is the IPMI port of the BaseboardManagement.
// +kubebuilder:validation:MinLength=1
Port string `json:"port"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be an Int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept it as string because bmclib expects port as a string. But we can change it to int if that makes more sense 👍

Signed-off-by: Aravind Ramalingam <ramaliar@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2022

Codecov Report

Merging #4 (26f3ea5) into main (0732a9c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main       #4   +/-   ##
=======================================
  Coverage   54.46%   54.46%           
=======================================
  Files           4        4           
  Lines         112      112           
=======================================
  Hits           61       61           
  Misses         42       42           
  Partials        9        9           
Impacted Files Coverage Δ
controllers/baseboardmanagement_controller.go 67.03% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0732a9c...26f3ea5. Read the comment docs.

Copy link
Contributor

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

LGTM

@micahhausler micahhausler added the ready-to-merge Mergify: Ready for Merging label May 17, 2022
@mergify mergify bot merged commit 4a1c506 into tinkerbell:main May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Mergify: Ready for Merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants