Dedicate command #896

khuong507 · 2017-11-22T19:55:35Z

No description provided.

coveralls · 2017-11-22T19:59:28Z

Coverage increased (+0.4%) to 85.989% when pulling a133971 on khuong507:dedicateCommand into dc97e7e on softlayer:master.

allmightyspiff

Overall looks good. My biggest concern is some of the hard coding around the dedicated host size stuff.

allmightyspiff · 2017-11-22T21:41:30Z

SoftLayer/CLI/dedicatedhost/create.py

+              help="Host portion of the FQDN",
+              required=True,
+              prompt=True)
+@click.option('--router', '-r',


unless its wildly complicated, I would prefer to pass in the router hostname, fcr02a.dal13 or something

Is specifying a VLAN not an option for dedicated hosts?

Specifying a VLAN is not an option for dedicated hosts

allmightyspiff · 2017-11-22T21:41:56Z

SoftLayer/CLI/dedicatedhost/create.py

+              default='hourly',
+              show_default=True,
+              help="Billing rate")
+@click.option('--test',


I'd prefer --verify, or --dry-run instead of test.

Currently virtual servers and hardware use --test. Would you still like it changed?

Yeah --test isn't too descriptive. I'd personally prefer --verify. Might want to make the help a little more descriptive to say it only verifies the dedicated host order without placing it.

allmightyspiff · 2017-11-22T21:47:32Z

SoftLayer/CLI/dedicatedhost/create.py

+        'hourly': args.get('billing') == 'hourly',
+    }
+
+    do_create = not (args['test'])


Since you are already testing this on line 55, just change line 84 to else:

allmightyspiff · 2017-11-22T21:50:24Z

SoftLayer/CLI/dedicatedhost/create.py

+    if args.get('test'):
+        result = mgr.verify_order(**order)
+
+        table = formatting.Table(['Item', 'cost'])


I think it would be good to include this pricing information in the order output as well.

allmightyspiff · 2017-11-22T21:52:40Z

SoftLayer/CLI/dedicatedhost/create_options.py

+    """host order options for a given dedicated host."""
+
+    mgr = SoftLayer.DedicatedHostManager(env.client)
+    options = mgr.get_create_options()


If its not too hard I'd also like to output the available routers (and vlans if possible) per DC here.

allmightyspiff · 2017-11-22T22:04:27Z

SoftLayer/managers/dedicated_host.py

+
+    def _get_item(self, package):
+        """Returns the item for ordering a dedicated host."""
+        description = '56 Cores X 242 RAM X 1.2 TB'


I can see this being problematic if we ever have more dedicated host sizes. I'd also prefer we check keynames not descriptions.
_get_item(self, package, itemKey='DEDICATED_HSOT')

or something like that.

allmightyspiff · 2017-11-22T22:08:49Z

SoftLayer/managers/dedicated_host.py

+            for location in locations:
+                if location['locationId'] is not None:
+                    loc_id = location['locationId']
+                    host = {


I can see this being problematic if more dedicated host sizes are added. This should be calculated from the package or supplied by the CLI I think.

Unless its possible to not supply this data at all and just get a list of routers without specifying a template

…tionality

coveralls · 2017-12-04T19:27:46Z

Coverage increased (+0.4%) to 86.03% when pulling 913e721 on khuong507:dedicateCommand into dc97e7e on softlayer:master.

rlrossiter

I've taken a first pass at this, mostly focusing on the create CLI and the manager. I'll take a deeper look at this later.

rlrossiter · 2017-12-04T20:50:38Z

SoftLayer/CLI/dedicatedhost/create.py

+              type=click.Path(writable=True, resolve_path=True),
+              help="Exports options to a template file")
+@environment.pass_env
+def cli(env, **args):


I know virt/create:cli does **args, but I find that really confusing since they're more conventionally **kwargs.

Though in my opinion, even though you do have a lot of options, I think it would be easier to read and see what options are all possibly going to be coming in on the cli by explicitly outlining all of the command line args that are coming in here.

rlrossiter · 2017-12-04T21:00:02Z

SoftLayer/CLI/dedicatedhost/create.py

+        'hostname': args['hostname'],
+        'domain': args['domain'],
+        'flavor': args['flavor'],
+        'router': args['router'],


Since router is optional, this may not be defined in the args. I think the @click.option decorator will make it a kwarg for you, so you don't need to worry about a key error here, but it will be set to None if it isn't set on the command line. Is that what this is supposed to be set to if no router is specified?

If not, I'd put the things you know will be set in the order (i.e. the mandatory items) in this initial dict creation, and then add things in later if needed, like:

if args['router']: order['router'] = args['router']

rlrossiter · 2017-12-04T21:00:53Z

SoftLayer/CLI/dedicatedhost/create.py

+        'domain': args['domain'],
+        'flavor': args['flavor'],
+        'router': args['router'],
+        'location': args.get('datacenter'),


this needs to be set doesn't it? It's required by the CLI, so it should just be args['datacenter']

.get() will return None if it isn't set, which you probably don't want to pass in the order

rlrossiter · 2017-12-04T21:03:15Z

SoftLayer/CLI/dedicatedhost/create.py

+              default='hourly',
+              show_default=True,
+              help="Billing rate")
+@click.option('--test',


Yeah --test isn't too descriptive. I'd personally prefer --verify. Might want to make the help a little more descriptive to say it only verifies the dedicated host order without placing it.

rlrossiter · 2017-12-04T21:07:05Z

SoftLayer/CLI/dedicatedhost/create.py

+    if len(result['prices']) != 1:
+        raise exceptions.ArgumentError("More than 1 price was found or no "
+                                       "prices found")
+    price = result['prices']


set price = result['prices'][0] here, because price[0] looks weird down below (and you're doing it twice so that helps clean up the below a little more)

rlrossiter · 2017-12-04T22:05:42Z

SoftLayer/CLI/hardware/create.py

        return

    if do_create:
+


I don't think you wanted to be changing this file

rlrossiter · 2017-12-04T22:07:46Z

SoftLayer/fixtures/SoftLayer_Product_Package.py

        'sort': 0
    }]
+
+verifyOrderDH = {


Not sure what the normal process is around adding fixtures, but this seems a little long. Do we want this reduced down so it only has the fields that you need? This question is more for @allmightyspiff

I don't really have a strong opinion on this at the moment.

rlrossiter · 2017-12-04T22:10:47Z

SoftLayer/managers/dedicated_host.py

+
+        kwargs['filter'] = _filter.to_dict()
+        func = getattr(self.account, call)
+        return func(**kwargs)


instead of making this roundabout way of calling getDedicatedHosts, I'd just use self.account.getDedicatedHosts(**kwargs)

rlrossiter · 2017-12-04T22:20:37Z

SoftLayer/managers/dedicated_host.py

+
+        raise SoftLayer.SoftLayerError("Could not find available routers")
+
+    def _get_default_router(self, routers, router_name):


I think for readability, since router_name isn't required, default it to None in the args here. Even though you always supply it above (and it's None because it's optional in _generate_create_dict()), I'd still be explicit with router_name=None

rlrossiter

Few more comments added from going through it a little closer.

rlrossiter · 2017-12-05T14:59:27Z

SoftLayer/CLI/dedicatedhost/create_options.py

+                   " ex. 56_CORES_X_242_RAM_X_1_4_TB",
+              show_default=True)
+@environment.pass_env
+def cli(env, **args):


Same for **kwargs here as in create. Though I would really just lean towards having datacenter and flavor as explicit kwargs for the function

rlrossiter · 2017-12-05T15:07:53Z

SoftLayer/CLI/dedicatedhost/create_options.py

+    else:
+        if args['flavor'] is None or args['datacenter'] is None:
+            raise exceptions.ArgumentError('Both a flavor and datacenter need '
+                                           'to be passed as arguments\n'


Don't do a '\n' in an exception string, just let it all flow on one line. If you want a separator for the example, maybe just put the example in parentheses

rlrossiter · 2017-12-05T16:03:59Z

SoftLayer/CLI/dedicatedhost/detail.py

+    table.add_row(['router hostname', result['backendRouter']['hostname']])
+    if utils.lookup(result, 'billingItem') != {}:
+        table.add_row(['owner', formatting.FormattedItem(
+            utils.lookup(result, 'billingItem', 'orderItem',


wouldn't this lookup just return {} if billingItem wasn't set, so the if above isn't needed? Because either way if something in that lookup isn't set, you'll end up with formatting.blank() anyways

rlrossiter · 2017-12-05T16:20:02Z

SoftLayer/managers/dedicated_host.py

+                'createDate,'
+                'modifyDate,'
+                'backendRouter[id, hostname, domain],'
+                'billingItem[id, nextInvoiceTotalRecurringAmount, '


I'd think about reorganizing this to make it a little more readable, because it's not obvious the nesting structure of this mask (i.e. children and orderItem are under billingItem, but the brackets are hard to follow)

Maybe look at how masking is done where a docstring is used, where different indentation can be used to make nesting easier to read. The only problem with the docstring method is printing it out in the debugger can get a little difficult to read.

coveralls · 2017-12-06T18:45:56Z

Coverage increased (+0.4%) to 86.03% when pulling a09d814 on khuong507:dedicateCommand into 7858dbd on softlayer:master.

coveralls · 2017-12-11T16:02:18Z

Coverage increased (+0.6%) to 86.214% when pulling 2abc785 on khuong507:dedicateCommand into 7858dbd on softlayer:master.

khuong507 · 2017-12-12T14:56:57Z

I made the requested changes.

allmightyspiff

Ok looks good. I'll do some personal testing with this tomorrow and if everything works as expected I'll be ready to merge into master, and released some point next week.

allmightyspiff · 2017-12-14T21:49:25Z

SoftLayer/fixtures/SoftLayer_Product_Package.py

        'sort': 0
    }]
+
+verifyOrderDH = {


I don't really have a strong opinion on this at the moment.

allmightyspiff · 2017-12-15T17:11:07Z

$ ./slcli  dedicatedhost create -H dedicatedHostTest -D cgallo.com -d  mex01 -f 56_CORES_X_242_RAM_X_1_4_TB --verify
Total hourly cost  0.00
 -- ! Prices reflected here are retail and do not take account level discounts and are not guaranteed.


./slcli  dedicatedhost create -H dedicatedHostTest -D cgallo.com -d  mex01 -f 56_CORES_X_242_RAM_X_1_4_TB
This action will incur charges on your account. Continue? [y/N]: y
Total hourly cost  0.00
 -- ! Prices reflected here are retail and do not take account level discounts and are not guaranteed.
id       21462711
created  2017-12-15T10:54:15-06:00


 ./slcli  dedicatedhost list
50701  ajcb-test          56  1200  242  mex01  1
10201  dedicated2         56  1200  242  dal10  NULL
64801  dedicatedHostTest  56  1200  242  mex01  NULL
10405  fmirDH             56  1200  242  dal09  1

Khuong-Nguyen added 7 commits November 6, 2017 12:59

VIRT-4404 : adding list functionality to dedicated host

9661c63

VIRT-4404 : Adding dedicated host functionality

1a48944

VIRT-4404 : Adding dedicated host functionality

aa944c7

VIRT-4404 : Adding Functionality for dedicated host

91376d7

VIRT-4404 : Adding Functionality for dedicated host

3a39d82

VIRT-4404 : fixed merge conflicts

11b6e9a

VIRT-4404 : Adding details functionality

a133971

allmightyspiff requested changes Nov 22, 2017

View reviewed changes

allmightyspiff self-assigned this Nov 22, 2017

allmightyspiff requested a review from rlrossiter November 22, 2017 22:12

Virt-4404 : Made suggested code changes and added dedicated host func…

913e721

…tionality

rlrossiter suggested changes Dec 4, 2017

View reviewed changes

rlrossiter reviewed Dec 5, 2017

View reviewed changes

Merge branch 'master' into dedicateCommand

a09d814

Added Dedicated host functionality

2abc785

allmightyspiff approved these changes Dec 14, 2017

View reviewed changes

allmightyspiff merged commit ea3edff into softlayer:master Dec 15, 2017

khuong507 deleted the dedicateCommand branch October 31, 2018 00:54


		raise SoftLayer.SoftLayerError("Could not find available routers")

		def _get_default_router(self, routers, router_name):

Dedicate command #896

Dedicate command #896

Uh oh!

Conversation

khuong507 commented Nov 22, 2017

Uh oh!

coveralls commented Nov 22, 2017

Uh oh!

allmightyspiff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 4, 2017

Uh oh!

rlrossiter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlrossiter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 6, 2017

Uh oh!

coveralls commented Dec 11, 2017

Uh oh!

khuong507 commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allmightyspiff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allmightyspiff commented Dec 15, 2017

khuong507 commented Dec 12, 2017 •

edited

Loading