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 ansible playbook runner to ansiblegate module #48254

Merged
merged 17 commits into from Jul 13, 2018

Conversation

gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Jun 21, 2018

What does this PR do?

Adds the ability to run ansible playbooks from salt execution modules or from states, specifically for orchestration states.

Tests written?

Added

Commits signed with GPG?

Yes

@gtmanfred gtmanfred added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 21, 2018
Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

Just one minor change I would make, otherwise this is ++.

if ansible_kwargs is None:
ansible_kwargs = {}
checks = __salt__['ansible.playbooks'](name, rundir=rundir, check=True, diff=True, **ansible_kwargs)
if all(not check['changed'] for _, check in six.iteritems(checks['stats'])):
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use six.itervalues() here and not need to use the "throwaway" variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool

@gtmanfred
Copy link
Contributor Author

Still need to write tests for this.

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

Generally, if we want to validate the params, we should do it explicitly. Otherwise I would use Pythonic checks (see the comment).

Otherwise, excellent, woohoo!! 😎

salt 'ansiblehost' ansible.playbook playbook=/srv/playbooks/play.yml
'''
command = ['ansible-playbook', playbook]
if check is True:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to explicitly check the type alongside the value? I.e. why if check: is not sufficient?

if diff is True:
command.append('--diff')
if extra_vars is not None:
command.append('--extra-vars={0}'.format(json.dumps(extra_vars)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, still the same question though. I'd say if extra_vars:

@gtmanfred gtmanfred changed the title [WIP] add ansible playbook runner to ansiblegate module add ansible playbook runner to ansiblegate module Jun 21, 2018
@gtmanfred gtmanfred removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jun 21, 2018
@gtmanfred
Copy link
Contributor Author

Test added

@gtmanfred
Copy link
Contributor Author

re-run py

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

Cool, thanks! Some last little nitpicks though.

'comment': 'Running playbook {0}'.format(name),
'name': name,
}
if gitrepo is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

If gitrepo is an empty string this will fail.

**git_kwargs,
)
if ansible_kwargs is None:
ansible_kwargs = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is better way, unless we do not want to validate all that and crash with the error "You've passed an empty string instead of a parameters" (or something like that):

if not ansible_kwargs:
    ansible_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.

I am just going to check if it a dict and if it isn't then i will set it to {}.

@rallytime
Copy link
Contributor

@gtmanfred
Copy link
Contributor Author

This should be all set to go on the next test run.

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

Is gitrepo part of a naming convention used elsewhere? If not, it may make sense to change it. I'm just worried about confusion between gitrepo (no underscore) and git_kwargs (underscore). It seems like this could lead to confusion/typos.

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

@terminalmage actually a very good catch, it slipped out of my radar: the git_<something> is better convention than git<something>.

@gtmanfred could you please appease BOFH (and terminalmage) by looking into name conventions and checking to None one more time? 😉 I would appreciate that.

Other then that, it is good to go as of my current views.

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

LOL 😆 👍 good to go.

@rallytime
Copy link
Contributor

@gtmanfred Lint :)

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

@gtmanfred docs, tests. :)

run nginx install:
ansible.playbooks:
- name: install.yml
- gitrepo: git://github.com/gituser/playbook.git
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bah... i did a search replace... apparently i am just terrible at regex.

ret = self.run_state(
'ansible.playbooks',
name='install.yml',
gitrepo='git://github.com/gtmanfred/playbooks.git',
Copy link
Contributor

@isbm isbm Jun 26, 2018

Choose a reason for hiding this comment

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

Tests. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

@gtmanfred thanks! Hopefully now it is OK to go.

@rallytime
Copy link
Contributor

@gtmanfred
Copy link
Contributor Author

re-run py

@gtmanfred
Copy link
Contributor Author

One last test to fix. Working on it this morning

@gtmanfred
Copy link
Contributor Author

Ok, this should be ready.

@rallytime
Copy link
Contributor

@gtmanfred
Copy link
Contributor Author

re-run py

@gtmanfred
Copy link
Contributor Author

re-run all

@rallytime
Copy link
Contributor

re-run py

@rallytime
Copy link
Contributor

@gtmanfred I think the following test is related. I am not seeing this fail elsewhere:

  • integration.modules.test_cmdmod.CMDModuleTest.test_run

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu16-py3/11261/

@rallytime rallytime merged commit 490d7fb into saltstack:develop Jul 13, 2018
@gtmanfred gtmanfred deleted the ansible branch July 13, 2018 19:29
@gtmanfred
Copy link
Contributor Author

🎉

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

4 participants