machines.get_power_parameters should not be top level on machines

[blakerouse]

We are creating this library to make it easy for people to use the MAAS API. Placing this operation at the top level doesn't seem like the best idea or present a clean library for people to use. I would think something like this would be better:

machine = await client.machines.get("asdasd")
power_parameters = await machine.get_power_parameters()

Getting the power parameters for all the nodes at the same time seems like a weird operation to want. If we want to have the top level one as well that is fine, but I think the main way and the documented way should really be, what is above.

[allenap]

Perhaps we should expose BMCs instead of the power parameters directly?

[blakerouse]

I would actually like this but the API at the moment does not allow this and would be some work to enable a BMC endpoint in the API. I think it would be best to add a get_power_parameters method to the machine object, and rename the other endpoint to machines.get_power_parameters_for.

[brendan-donegan]

I don't think get_power_parameters_for makes any sense in the context of getting power params for all machines, it reads like 'get power parameters for nothing'

I think users will understand that in the context of 'machines' rather than a 'machine', it gets all power parameters (or the specified ones)

[blakerouse]

What is weird about the get_power_parameters on machines is that it doesn't return a Machine object. It returns a list of dictionaries that have no correlation to the machine that those parameters apply to.

The get_power_parameters_for is the symbol that you should provide a list of machines you want the power parameters. I think its wierd in this case that our API returns all for all machines. I think this API should return an empty list unless system_id are given.

[brendan-donegan]

But then there's no way to get all of the power parameters, should you wish to do that - you would have to iterate over each machine, or get all the machines then construct a list of system_ids and call get_power_parameters_for with that

On Wed, 15 Mar 2017 at 19:29 Blake Rouse notifications@github.com wrote:

What is weird about the get_power_parameters on machines is that it doesn't return a Machine object. It returns a list of dictionaries that have no correlation to the machine that those parameters apply to.

The get_power_parameters_for is the symbol that you should provide a list of machines you want the power parameters. I think its wierd in this case that our API returns all for all machines. I think this API should return an empty list unless system_id are given.

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/maas/python-libmaas/issues/84#issuecomment-286853320, or mute the thread https://github.com/notifications/unsubscribe-auth/ACS2LnmGsgLZ7WUyAOPlpCBrxzMGgzZyks5rmDwXgaJpZM4Md5Ux .

[blakerouse]

Why would you ever want all the power_parameters with no context of what machines you want them for? Seems weird to say just give me all the power_parameters? Why for what?

I am also not saying that get_power_parameters_for is not useful, just is this the correct way to do it.

[brendan-donegan]

client.machines.get_power_parameters() client.machines.get_power_parameters(system_ids=[...])

vs.

client.machines.get_power_parameters_for() client.machines.get_power_parameters_for(system_ids=[])

To me, the first one is slightly less clear when a subset of the power params, but still clear enough and much clearer when getting all of them. The second is slightly more clear for the subset, but actively confusing when we intend to get them all.

I do agree that getting a subset is going to be the more common usage though.

Another proposal:

client.machines.get_all_power_parameters() # takes no arguments client.machines.get_power_parameters_for(system_ids=[...]) # takes 1 required argument

Clear in both cases, but slightly less elegant (not that there's anything elegant about any of this)

On Wed, 15 Mar 2017 at 19:34 Brendan Donegan brendan.j.donegan@gmail.com wrote:

But then there's no way to get all of the power parameters, should you wish to do that - you would have to iterate over each machine, or get all the machines then construct a list of system_ids and call get_power_parameters_for with that

On Wed, 15 Mar 2017 at 19:29 Blake Rouse notifications@github.com wrote:

What is weird about the get_power_parameters on machines is that it doesn't return a Machine object. It returns a list of dictionaries that have no correlation to the machine that those parameters apply to.

The get_power_parameters_for is the symbol that you should provide a list of machines you want the power parameters. I think its wierd in this case that our API returns all for all machines. I think this API should return an empty list unless system_id are given.

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/maas/python-libmaas/issues/84#issuecomment-286853320, or mute the thread https://github.com/notifications/unsubscribe-auth/ACS2LnmGsgLZ7WUyAOPlpCBrxzMGgzZyks5rmDwXgaJpZM4Md5Ux .

[blakerouse]

I like the final proposal the best as its explanatory, but would like to get @allenap opinion on the matter as well.

Also what do these calls return? Just dictionaries? Do each reference a system_id some how? Should we expose the result as a first class object?

[brendan-donegan]

I know that the machines one returns a dict of dicts, keyed by system_id (as per the HTTP API), but I'm not sure about the machine one, it might return a dict of dicts as well, which would be wrong - it should return just a dict. As for wrapping them, I did suggest that initially when the first bug was filed but we (me and Gavin) ended up deciding against it. Maybe we can rethink it.

On Wed, 15 Mar 2017 at 20:13 Blake Rouse notifications@github.com wrote:

I like the final proposal the best as its explanatory, but would like to get @allenap https://github.com/allenap opinion on the matter as well.

Also what do these calls return? Just dictionaries? Do each reference a system_id some how? Should we expose the result as a first class object?

— You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub https://github.com/maas/python-libmaas/issues/84#issuecomment-286864390, or mute the thread https://github.com/notifications/unsubscribe-auth/ACS2LhfmYd2dXRahQJP-eX-Q2hwNzfbzks5rmEZNgaJpZM4Md5Ux .

[blakerouse]

Also the more I think about get_all_power_parameters I feel like the all is in the wrong place. It feels like all parameters not parameters for all machines. So maybe get_power_parameters_for_all would be better.

I think it would be best for the client.machines to return a dictionary of unloaded Machine objects with the values being the power_parameters dictionary. As for the method on the Machine object it should just be a dictionary of power_parameters which is what the API returns.

machines_params = await machines.get_power_parameters_for_all()
machine, params = machine_params.popitem()
assert isinstance(machine, Machine)
assert machine.system_id is not None
assert machine.loaded is False
assert isinstance(params, dict)

# From this point the machine can be loaded.
await machine.load()
assert machine.loaded is True

[blakerouse]

The load() and loaded is documented in the library specification that we designed. Don't know if we are close to having that implemented yet or not.

[brendan-donegan]

Ok - if we are going with those method names then I will implement them so that they return a dictionary keyed by unloaded Machines

[brendan-donegan]

Actually, can the Machine object be keys? I would have thought they are mutable?

[blakerouse]

Anything can be a dictionary key if its hashable. The hash key for Machine should be the system_id.

[allenap]

My thoughts:

• Don't expose a get-power-parameters-for-all-machines operation. It's not clear we need it now.

• I remain reluctant to embrace the partially-loaded-object pattern, e.g. where a Machine instance knows only its system_id and the remaining attributes must be vivified by a call to .load(). That's a longer discussion, but here I think we should avoid that pattern until we've had it.

• We could/should note in the docstrings that these methods remain "alpha" and may change once we model BMCs as first class objects.

Hence I suggest something fairly minimal:

Machines.get_power_parameters_for([...])  # --> {system_id: {dict-of-parameters}, ...}
Machine.get_power_parameters()  # --> {dict-of-parameters}

[brendan-donegan]

One last thing that's not 100% clear for me - should calling get_power_parameters_for() with no arguments (or indeed an empty list), return all power parameters, as a sort of 'easter egg'. Otherwise I'll need to write some code to 'defend' against calling it that way.

[allenap]

Let's say:

get_power_parameters_for()  # --> TypeError
get_power_parameters_for([])  # --> {}

The TypeError comes from Python when a mandatory argument is not supplied; it doesn't need to be raised explicitly.

[blakerouse]

@allenap - I think avoiding the pattern is not a good idea. When we present this in the library I think we should strive to make it the way we want it to be. So we need to come to a decision on that pattern very soon. Without that pattern or another pattern that we determine, we have no nested objects. MAAS has lots of nested objects.