-
Notifications
You must be signed in to change notification settings - Fork 236
OCM-20977 | fix: add missing capacity-reservation-preference field in 'rosa describe machibepool' #3192
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?
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olucasfreitas The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
… 'rosa describe machibepool'
e8cfba3 to
d5bade2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3192 +/- ##
==========================================
+ Coverage 23.97% 24.05% +0.07%
==========================================
Files 329 329
Lines 35908 35916 +8
==========================================
+ Hits 8609 8639 +30
+ Misses 26646 26623 -23
- Partials 653 654 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| id, ok := capacityReservation.GetId() | ||
| if !ok { | ||
| return "" | ||
| id, hasId := capacityReservation.GetId() |
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.
out of convention, I would like hasId to become ok 🙂 its just a convention that we use
| preference, ok := capacityReservation.GetPreference() | ||
| if !ok { | ||
| return fmt.Sprintf("\n"+ | ||
| " - ID: %s\n"+ | ||
| " - Type: %s", | ||
| id, marketType) | ||
| } | ||
| return fmt.Sprintf("\n"+ | ||
| " - ID: %s\n"+ | ||
| " - Type: %s\n"+ | ||
| " - Preference: %s", | ||
| id, marketType, preference) | ||
| } |
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.
it looks like we already have preference added here, I think the issue might be more complicated than just 'we forgot it'
It looks like we are returning early before returning this block with the preference, I think the right solution for this ticket would be to fix the issue directly, rather than stack on a couple new if statements and a return
Likely, one of the above returns is being used before this one is, causing the preference to not be visible, and we should figure out why
Also there's quite a few tests introduced in this MR, I think what would be important would be to directly test the outcome, so keep the cmd_test.go changes, but we don't need anything else beyond that, unless a test fails (in that case, we should modify it)
Details
This PR adds the
capacity-reservation-preferencefield to the output ofrosa describe machinepool, so users can see what preference was set on their node pool.Fixed Behavior
Output:
Now the user can see the preference they configured.
Additional Cases
The existing output for capacity reservations with an ID continues to work as before:
ID + Type (no preference):
ID + Type + Preference:
Preference only (new):
Ticket
Closes OCM-20977