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

New Command: slcli virtual host-list #1835

Merged
merged 8 commits into from
Feb 1, 2023

Conversation

BrianSantivanez
Copy link

Issue: #1829
Observations: virtual host-list command was added with its unit tests

slcli vs host-list --order 85857762 -n dedicatedhost01 -d dal13 --owner sl307608-dcabero
┌────────┬─────────────────┬────────────┬──────────────┬───────────────────────┬──────────────────────────┬────────────────────────┬────────┐
│   Id   │ Name            │ Datacenter │    Router    │ CPU (allocated/total) │ Memory (allocated/total) │ Disk (allocated/total) │ Guests │
├────────┼─────────────────┼────────────┼──────────────┼───────────────────────┼──────────────────────────┼────────────────────────┼────────┤
│ 656700 │ dedicatedhost01 │   dal13    │ bcr01a.dal13 │         0/56          │          0/242           │         0/1200         │   0    │
└────────┴─────────────────┴────────────┴──────────────┴───────────────────────┴──────────────────────────┴────────────────────────┴────────┘

@edsonarios
Copy link
Contributor

Screenshots of the test result
image
image

SoftLayer/managers/account.py Outdated Show resolved Hide resolved
tests/managers/account_tests.py Outdated Show resolved Hide resolved
SoftLayer/CLI/virt/host_list.py Outdated Show resolved Hide resolved
@allmightyspiff allmightyspiff linked an issue Jan 25, 2023 that may be closed by this pull request
Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

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

I hate to waste all the work you did, but we already have slcli dedicatedhost list, and I don't want to duplicate that code here.

Do add an alias for slcli vs host-list like I mentioned in the routes.py file though. I just want both commands to use the same code.

Add the order and owner flags to slcli dedicatedhost list. I'd prefer that the dedicatedhost manager build the actual filter though.

I'd also like to take this time to remove the cpu, memory, disk filters from dedicatedhost list since I don't think they add much value. I'd also like to change the table it creates to be like the one you are building in this change. We can also remove the --columns option from the list command as well.

TODO

  • Have slcli vs host-list route to SoftLayer.CLI.dedicatedhost.list
  • Add order and owner options to slcli dedicatedhost list
  • Remove cpu, memory, disk and columns options from slcli dedicatedhost list
  • Update table to be the one you created in this pull request
  • Undo updates you made to the account manager, since we will be using the dedicatedhost managers list_instances function instead.

I hope that all makes sense. Thanks

SoftLayer/CLI/routes.py Outdated Show resolved Hide resolved
docs/cli/vs.rst Outdated Show resolved Hide resolved
SoftLayer/CLI/dedicatedhost/list.py Outdated Show resolved Hide resolved
Copy link
Member

@allmightyspiff allmightyspiff left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just address Daniel's comment and this will be good to go. Thanks

Copy link
Contributor

@caberos caberos left a comment

Choose a reason for hiding this comment

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

LGTM

@allmightyspiff allmightyspiff merged commit e63d10a into softlayer:master Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

New Command: slcli virtual host-list
4 participants