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

update goss verify.yml to allow to pass host to run on #1646

Merged
merged 15 commits into from
Apr 9, 2019

Conversation

loomsen
Copy link
Contributor

@loomsen loomsen commented Jan 2, 2019

This PR allows for the goss verifier to run different tests on certain hosts.
Example: have a side-effect playbook, which makes keepalived move an IP to a different host in an active-active scenario. The verifier should verify that the IP actually moved to a different host, and that the side-effect playbook killed the supervised process.

Basically what you can do with testinfra by specifying the host in .get_hosts

testinfra_hosts = testinfra.utils.ansible_runner.AnsibleRunner(
    os.environ['MOLECULE_INVENTORY_FILE'])\
    .get_hosts('instance1')

You can do this for goss by prepending the test file with test_host_$hostname_as_given_in_molecule.yml.

PR Type

  • Feature Pull Request

@loomsen loomsen force-pushed the master branch 2 times, most recently from b456261 to cd49513 Compare January 2, 2019 18:11
@loomsen loomsen closed this Jan 3, 2019
@loomsen loomsen reopened this Jan 3, 2019
Copy link
Contributor

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm finding it hard to understand your use case. Does this change cover a general case? Can you explain further? Are there any tests you can provide here?

loomsen pushed a commit to loomsen/ansible-nginx-ha-cluster-pr-example that referenced this pull request Jan 16, 2019
@loomsen
Copy link
Contributor Author

loomsen commented Jan 16, 2019

Hi, thank you for reviewing my PR :)

I set up a stripped down, real world example of the roles we actually use in production. It's dumbed down to a default vhost returning 502, but it shows the purpose of this PR.

https://github.com/loomsen/ansible-nginx-ha-cluster-pr-example

Another scenario would be a master-slave databse setup, where you want to setup the replication with ansible, and have a side effect playbook run to make sure the failover is working.
Without this PR, there is no way to test that, because there is only one common goss test file for all hosts.

I hope this enlightens the dark. Does it?

Norbert Varzariu added 6 commits January 16, 2019 16:26
Signed-off-by: Norbert Varzariu <[email protected]>
Signed-off-by: Norbert Varzariu <[email protected]>
Signed-off-by: Norbert Varzariu <[email protected]>
Signed-off-by: Norbert Varzariu <[email protected]>
@loomsen
Copy link
Contributor Author

loomsen commented Jan 16, 2019

OK I changed ignore_errors: true to failed_when: false, which seems a little prettier.
Pls review.

@loomsen loomsen closed this Jan 16, 2019
@loomsen loomsen reopened this Jan 16, 2019
@loomsen
Copy link
Contributor Author

loomsen commented Jan 16, 2019

Politely asking travis for a rebuild, the failure seems kind of random to me.

@loomsen loomsen closed this Jan 16, 2019
@loomsen loomsen reopened this Jan 16, 2019
@loomsen
Copy link
Contributor Author

loomsen commented Jan 16, 2019

Yeah, those seem like random failures, now it failed at a different step.

@loomsen loomsen closed this Jan 18, 2019
@loomsen loomsen reopened this Jan 18, 2019
@decentral1se
Copy link
Contributor

Sorry that you're PR is sitting so long here @loomsen. Thanks for the further example.

I've given some more time on this. If I understand it correctly, this PR solves two issues:

So yes, this definitely feels like something we need then. This PR is missing documentation for this change that would definitely make it more visible for other users. Also, can me perhaps use the more readable find module to replace the shell: ... pattern matching?

And finally, how to test this? It will be hard to have any sense of sureness without that. If there isn't something already available to help you write tests, please let us know and we better get to making that happen too!

@loomsen
Copy link
Contributor Author

loomsen commented Jan 23, 2019

depends on #1695

@loomsen
Copy link
Contributor Author

loomsen commented Jan 23, 2019

Only linter fails, which is addressed in #1695

Copy link
Contributor

@decentral1se decentral1se left a comment

Choose a reason for hiding this comment

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

This is looking great now @loomsen!

@loomsen
Copy link
Contributor Author

loomsen commented Jan 25, 2019

I don't know why the builds fail.
https://travis-ci.com/loomsen/molecule/builds/98478819
That's the build from my fork, which is fine (except the lint errors).

After fetching latest master into my develop branch, some jobs also start failing.

https://travis-ci.com/loomsen/molecule/builds/98581045

Hence I think the failures are unrelated to my changes.
What do you guys think?

@decentral1se
Copy link
Contributor

Added to agenda in ansible/community#427. Please join if you have time!

@themr0c themr0c added this to the v.2.21 milestone Jan 31, 2019
@decentral1se decentral1se modified the milestone: v.2.21 Jan 31, 2019
@decentral1se
Copy link
Contributor

Thanks for your patience @loomsen. Our CI is back in shape. It reported:

https://travis-ci.com/ansible/molecule/jobs/179293976

Warning, treated as error:
/home/travis/build/ansible/molecule/molecule/verifier/goss.py:docstring of molecule.verifier.goss.Goss:51:Error in "code-block" directive:
maximum 1 argument(s) allowed, 18 supplied.
.. code-block::
    test_default.yml (will run on all hosts)
    test_host_instance1.yml (will run only on instance1)
    test_host_instance2.yml (will run only on instance2)
ERROR: InvocationError for command '/home/travis/build/ansible/molecule/.tox/doc/bin/python setup.py build_sphinx -n -W --builder=html' (exited with code 1)

@decentral1se decentral1se removed this from the v.2.21 milestone Feb 27, 2019
@decentral1se
Copy link
Contributor

Just missing the --signoff DCO for the commit 😰

@loomsen
Copy link
Contributor Author

loomsen commented Mar 11, 2019

Gah, every single time.

- add trailing dot to goss bin shasum to follow current style

Signed-off-by: Norbert Varzariu <[email protected]>
Norbert Varzariu added 2 commits March 11, 2019 21:05
Signed-off-by: Norbert Varzariu <[email protected]>
@loomsen
Copy link
Contributor Author

loomsen commented Mar 12, 2019

Woohoo, finally :)

@decentral1se
Copy link
Contributor

Oh no .... we still didn't merge this ....

@loomsen, can you resolve the final merge conflicts? (sorry 🙏).

We're waiting on one final issue wrt. packaging and then we can merge this (for real).

😅

@loomsen
Copy link
Contributor Author

loomsen commented Apr 9, 2019

OK, so I pulled in the latest changes. Code style check fails, which shouldn't be related to this PR, and also a test randomly fails at destroy stage, maybe one of you could re-run just this build, if possible?

@decentral1se thanks for bearing with me :)

@decentral1se
Copy link
Contributor

No, thank you!!!

@decentral1se
Copy link
Contributor

decentral1se commented Apr 9, 2019

It appears to be related to #1911!

I'm re-running the CI and will merge if it passes 👍

@decentral1se
Copy link
Contributor

@decentral1se decentral1se merged commit 9ffa4ea into ansible:master Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants