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

Virt capabilities #47892

Merged
merged 2 commits into from Jun 12, 2018
Merged

Virt capabilities #47892

merged 2 commits into from Jun 12, 2018

Conversation

cbosdo
Copy link
Contributor

@cbosdo cbosdo commented May 30, 2018

What does this PR do?

Add the virt.capabilities() and virt.domain_capabilities() functions. These are needed by client applications to compute proper XML definitions for virtual guests.

What issues does this PR fix or reference?

None know so far.

Tests written?

Yes

Commits signed with GPG?

Yes

return result


def capabilities(**kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't typically like to accept **kwargs in execution modules. Would these typically be passed in by the user and if so, how would they know what to put in the dictionary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cachedout these are only used to allow user to provide connection details to override the minion config. These are documented in the module top level documentation, introduced by commit 36b1d19. I agree no other parameter should be passed using **kwargs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cachedout should I rather add connection=None, username=None, password=None parameters to almost all functions in that module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose the **kwargs way since there was already some used in a few functions in virt module and it saves duplicating three parameters and their doc in each and every function of the module... I think it help focusing on the real thing the function does and reduces the amount of visual noise in the module... and hopefully the number of bugs to be found later one.

But if you guys really don't want **kwargs at all I can change to use the 3 parameters: it's a boringly repetitive task, but I don't mind.

Copy link
Contributor

@isbm isbm Jun 12, 2018

Choose a reason for hiding this comment

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

@cachedout actually you are right, we in Salt tend to use explicitly enumerated args, that's true. But on the other hand, this brings more code when it needs to be repeated all the time across the entire module.

However I personally think that the right docstring is sufficient in this #case. In Python all o the same named arguments, and so you can call it as this:

def foo(name, second=None, **kwargs):
      return name, second

assert foo('Salt', 12345) == foo(**{'name': 'Salt', 'second': 1245,
                                    'something': 'else'})

But if you think this should be explicit, well, then it should be so. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added documentation for those parameters in the added functions. I'll submit a fixup commit for the already existing functions once all the PRs for virt module are merged to avoid rebase mess.

return True
elif supported == 'no':
return False
return None
Copy link
Contributor

@isbm isbm Jun 11, 2018

Choose a reason for hiding this comment

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

SESE, please?

Also when you say "return a boolean value" in the description, yet you make it like an old Pentium CPU: a) "true", b) "false" and c) "I don't know" (None). 😉

Hence I would write it as much as this:

def _parse_caps_supported(node):
    return node.getAttribute('supported') == 'yes'

But if this is just a check, do we need the whole function at all??

values = []
for value in node.getElementsByTagName('value'):
values.append(_get_xml_element_text(value))
return (name, values)
Copy link
Contributor

@isbm isbm Jun 11, 2018

Choose a reason for hiding this comment

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

This is why SESE matters. You return different types here. Once just None, other case tuple. That said, you cannot anymore do something like:

name, values = _parse_caps_enum(node)

Once you did, it will crash, in case node has no attribute name. So I would write it this way:

def _parse_caps_enum(node):
    return (node.getAttribute('name') or None,
            [_get_xml_element_text(value) for value in node.getElementsByTagName('value')])

for child in node.childNodes:
if child.nodeType == xml.dom.Node.TEXT_NODE:
text = text + child.data
return text
Copy link
Contributor

Choose a reason for hiding this comment

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

No += on strings, please! At least this:

def _get...
    text = []
    for child...
        if child.nodeType == xml.dom.Node.TEXT_NODE and child.data:
            text.append(child.data)
return [' '].join(text)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@isbm Thanks for the review, just pushed changes to address that and use more list/dict comprehensions to get code simpler

@isbm
Copy link
Contributor

isbm commented Jun 11, 2018

@cbosdo did you push anything?.. Because I want to re-check, but I see no changes yet.

@cbosdo cbosdo force-pushed the virt-capabilities branch 2 times, most recently from 7a630f1 to adcc9da Compare June 12, 2018 11:56
@cbosdo
Copy link
Contributor Author

cbosdo commented Jun 12, 2018

@isbm yes I did, just seen that I forgot one of the most visible occurrences of += :) Now all should be fine

nodes = node.getElementsByTagName(name)
if nodes:
return nodes[0]
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

return node and nodes[0] or None

😉 If you find this badly read, here is a kosher Python way:

return nodes[0] if node else None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went the second way... it's less cryptic at first sight ;)

name = canonical

machine = machines.get(name)
if machine is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be machine == '' or machine == 0? If that is not the case, then if not machine: is the way to go here.

Connection capabilities are useful for client applications to compute
XML definitions that will work on the hypervisor. This commit adds
a virt.capabilities() function parsing and returning those capabilities.
Client applications will need both the connection and domain
capabilities to compute proper domain XML definitions. Connection
capabilities were already implemented, this commit now adds a
virt.domain_capabilities function to fill the gap.
@rallytime rallytime merged commit b7ba930 into saltstack:develop Jun 12, 2018
@cbosdo cbosdo deleted the virt-capabilities branch August 2, 2018 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants