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

Adding support for RH and validate checksum #12

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Adding support for RH and validate checksum #12

wants to merge 3 commits into from

Conversation

dj-wasabi
Copy link

Made the role also work on RedHat systems. Also checking if MD5sum is correct.

@mtchavez
Copy link
Owner

I'd like to get some kind of testing setup for the different platforms before being confident these changes are good and don't break anything. I also would like to see the tasks split up more logically by including OS specific task files based on the fact lookup of OS type. I think this pattern is the convention for ansible roles and I'd like it to be split up before both supporting multiple platforms and merging this in.

On a styling note the whole role uses the jinja formatting of {{var_name}} without the white space padding on either side of the variable name. For consistency please remove the extra white space.

Overall this is in the right direction and just haven't spent the time to do it myself so thanks for getting something started with this.

@dj-wasabi
Copy link
Author

Hi @mtchavez

I'll remove the white spaces, but you are aware that this is the Ansible default guidelines with the spaces?

What I do with my Ansible roles is using Test kitchen. I've written an blogpost about this:
http://werner-dijkerman.nl/2015/08/20/using-test-kitchen-with-docker-and-serverspec-to-test-ansible-roles/ and http://werner-dijkerman.nl/2016/01/15/extending-the-ansible-test-kitchen-tests-with-bats-tests/

But this will only work local or on some server you own with tools like Jenkins/Hudson or other CI. There is no (public) cloud provider like travis that can work with this (As far that I know). If you need any help, please let me know.

Kind regards,
Werner

@mtchavez
Copy link
Owner

The current style is my personal preference. For consistency, I'd like to keep it all the same regardless of the style chosen and if there is a change to convert all the variable interpolation to have padding then that is fine as long as it is consistent.

@mtchavez
Copy link
Owner

I am aware of test kitchen, server spec, bats etc. Maybe I'm being optimistically naive here in thinking that these technologies just use tools like Vagrant and Docker to spin up VMs and containers to provision and test against. Nothing inhibits setting these dependencies up on things like Travis or CircleCI and running tests.

@dj-wasabi
Copy link
Author

I just pushed the commit where I removed all the spaces in the variables.

@mtchavez
Copy link
Owner

So a lot of stuff has changed lately #17 was done to have consistent styling to for ease of use for open source contributing and Ansible standards, as you suggested. Also testing overhaul was done in #19 to get kitchen tests for platform specific provisioning. It's a little hacked for a Galaxy role. I want to reach out to the ansible-busser project and see what we can do to make it easier. For now this should be able to at least get your changes tested. If you want to take a stab at it let me know, otherwise I can use what you have done here and make a new branch and pull request and ping you for review. Let me know. Thanks for being patient on this.

@dj-wasabi
Copy link
Author

Hi @mtchavez

Sorry for the late answer, but will try again.

I get this eror on the master branch when I do an kitchen test:

>>>>>> ------Exception-------
>>>>>> Class: Kitchen::ActionFailed
>>>>>> Message: Failed to complete #converge action: [No hosts have been set. Please specify one in .kitchen.yml]
>>>>>> ----------------------
>>>>>> Please see .kitchen/logs/kitchen.log for more details
>>>>>> Also try running `kitchen diagnose --all` for configuration

zlib(finalizer): the stream was freed prematurely.

Do you have the same error? If so, I'll try to fix this with this PR as well.

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.

2 participants