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

add template version #53636

Open
wants to merge 9 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@mnecas
Copy link
Contributor

mnecas commented Mar 11, 2019

SUMMARY

@machacekondra I had some issues with setting version number, maybe you could see where I made mistake.

Fixes #53620

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ovirt

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 11, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_template.py:474:33: bad-whitespace Exactly one space required after comma         version=dict(default=None,type='dict'),                                  ^

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_template.py:474:34: E231 missing whitespace after ','

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_template.py:0:0: E309 version_added for new option (version) should be '2.8'. Currently StrictVersion ('2.9')

click here for bot help

base_template=otypes.Template(
name=self._module.params['version']['base_template']
) if self._module.params['version'].get('base_template') else None,
version_number=self._module.params['version']['number'],

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

please use here self.param('version').get('number')

@@ -315,6 +321,13 @@ def build_entity(self):
memory=convert_to_bytes(
self.param('memory')
) if self.param('memory') else None,
version=otypes.TemplateVersion(
base_template=otypes.Template(
name=self._module.params['version']['base_template']

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

please use here self.param('version').get('base_template')

version=otypes.TemplateVersion(
base_template=otypes.Template(
name=self._module.params['version']['base_template']
) if self._module.params['version'].get('base_template') else None,

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

please use here self.param('version').get('base_template')

name=self._module.params['version']['base_template']
) if self._module.params['version'].get('base_template') else None,
version_number=self._module.params['version']['number'],
version_name=self._module.params['version']['name'] if self._module.params['version'].get('name') else None,

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

please use here self.param('version').get('name')

) if self._module.params['version'].get('base_template') else None,
version_number=self._module.params['version']['number'],
version_name=self._module.params['version']['name'] if self._module.params['version'].get('name') else None,
)if self._module.params['version'] else None,

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

please use here self.param('version')

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

and please add space after )

@machacekondra
Copy link
Contributor

machacekondra left a comment

Code looks ok, just few formatting comments

version_number=self._module.params['version']['number'],
version_name=self._module.params['version']['name'] if self._module.params['version'].get('name') else None,
)if self._module.params['version'] else None,
name=self._module.params('version').get('base_template')

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

Please use self.param('') instead of self._module.params()

name=self.param('version').get('base_template')
) if self.param('version').get('base_template') else None,
version_number=self.param('version').get('number') if self.param('version').get('number') else None,
version_name=self.param('version').get('name') if self.param('version').get('name') else None,

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

the if isn't needed here

base_template=otypes.Template(
name=self.param('version').get('base_template')
) if self.param('version').get('base_template') else None,
version_number=self.param('version').get('number') if self.param('version').get('number') else None,

This comment has been minimized.

@machacekondra

machacekondra Mar 12, 2019

Contributor

the if isn't needed here

@mnecas mnecas force-pushed the mnecas:ovirt_template_add_version branch from 2950352 to 5fdffe3 Mar 12, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 12, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_template.py:474:33: bad-whitespace Exactly one space required after comma         version=dict(default=None,type='dict'),                                  ^

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/cloud/ovirt/ovirt_template.py:474:34: E231 missing whitespace after ','

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Mar 12, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 18, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/modules/cloud/ovirt/ovirt_template.py:444:7: singleton-comparison Comparison to None should be 'expr is not None'
lib/ansible/modules/cloud/ovirt/ovirt_template.py:531:15: singleton-comparison Comparison to None should be 'expr is not None'

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/ovirt/ovirt_template.py:444:51: E711 comparison to None should be 'if cond is not None:'
lib/ansible/modules/cloud/ovirt/ovirt_template.py:531:41: E711 comparison to None should be 'if cond is not None:'

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Mar 18, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 19, 2019

@mnecas this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@mwperina
Copy link
Contributor

mwperina left a comment

Looks good to me. Ondro?

@mnecas mnecas force-pushed the mnecas:ovirt_template_add_version branch from 049e285 to 264460e Mar 19, 2019

version:
description:
- "C(name) - The name of this version."
- "C(base_template) - ID of the referenced template that this version is associated with."

This comment has been minimized.

@machacekondra

machacekondra Mar 20, 2019

Contributor

Can we use (also) name?

This comment has been minimized.

@machacekondra

machacekondra Mar 20, 2019

Contributor

Please add some example of how to use this parameter into EXAMPLES section.

resp = None
version = module.params.get('version')
if version.get('number') is not None:
templates = templates_service.list()

This comment has been minimized.

@machacekondra

machacekondra Mar 20, 2019

Contributor

Can you please add this logic to some method called find_template so you don't have to reimplenet the logic of update/create.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.