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

Ovirt add job #55440

Open
wants to merge 10 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@mnecas
Copy link
Contributor

commented Apr 17, 2019

SUMMARY

Ovirt add job module

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

Ovirt

ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

)


def build_job(module):

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

pass here just description, as it's the only parameter you need.

)


def attach_steps(module, entity_id, jobs_service):

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

here you need just a 'steps' not whole module as well.

)


def attach_steps(module, entity_id, jobs_service):

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

also please rename entity_id to job_id, no need to have it too generic here

ret = {
'changed': changed,
'id': getattr(job, 'id', None),
type(job).__name__.lower(): get_dict_of_struct(

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

change 'type(job).name.lower()' to just 'job' no need to do it generic here

type(job).__name__.lower(): get_dict_of_struct(
struct=job,
connection=connection,
fetch_nested=module.params.get('fetch_nested'),

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

I would add here fetch_nested=True, because we always want to return steps.

required: true
state:
description:
- "Should the setp be present/absent/failed."

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

s/setp/step

state:
description:
- "Should the job be present/absent/failed."
- "Present have alias started and absent have alias finished, you can use both."

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

please use C() around possible values of the state

state:
description:
- "Should the job be present/absent/failed."
- "Present have alias started and absent have alias finished, you can use both."

This comment has been minimized.

Copy link
@machacekondra

machacekondra Apr 18, 2019

Contributor

I would write: C(started) is alias for C(present). C(finished) is alias for C(absent). Same in the steps.

@ansibot ansibot removed the ci_verified label Apr 18, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@KKoukiou @derez @machacekondra @mwperina @nasx

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@machacekondra

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

shipit

@ansibot ansibot added shipit and removed community_review labels Apr 24, 2019

@mwperina
Copy link
Contributor

left a comment

shipit

@ansibot ansibot added community_review and removed shipit labels May 15, 2019

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.