machines.get_power_parameters should not be top level on machines about python-libmaas HOT 18 CLOSED

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

from python-libmaas.

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.

from python-libmaas.

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)

from python-libmaas.

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.

from python-libmaas.

from python-libmaas.

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.

from python-libmaas.

from python-libmaas.

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?

from python-libmaas.

from python-libmaas.

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

from python-libmaas.

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.

from python-libmaas.

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

from python-libmaas.

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

from python-libmaas.

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

from python-libmaas.

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}

from python-libmaas.

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.

from python-libmaas.

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.

from python-libmaas.

@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.

from python-libmaas.