-
Notifications
You must be signed in to change notification settings - Fork 3
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
CI: add more tests #13
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,28 @@ | |||
#!/usr/bin/env bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to add some short description of the purpose of the file and how to use it in 'must succes' and 'must fail scenarios'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion, added
- openssh-server | ||
state: present | ||
|
||
- name: Ensure sshd service is started |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be better to make it parametrized per distro/codename?
aloo 14.04 is really dead.
- name: Converge | ||
hosts: all | ||
vars: | ||
- theo_url: https://theo.example.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm super picky here, pleas double quote values, I've seen nightmares due to colons improperly parsed :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done
GCK9sSDVI5XY3wy6UYMo9SZQGIglyRPrnd3R82O277lAyOVC/NNp1vq5WH/Mi1Mu | ||
JK85kX7Atut+tgWgwuwT5vcCAwEAAQ== | ||
-----END PUBLIC KEY----- | ||
pre_tasks: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you added double newline between pre_tasks
and roles
, maybe adding single line between vars
and pre_tasks
could help visually here aswell?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but I think I prefer to remove the empty lines
|
||
MUST_FAIL="centos6:custom-config-file centos6:custom-config-dir" | ||
MUST_FAIL="centos6:custom-config-file centos6:custom-config-dir centos6:custom-all" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the feeling there is too much of must fail
scenarios. Anyway to fix this to actually make it pass, without getting isane ;-) ?
Or maybe just drop centos6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is to be sure that the role correctly fails in that cases so none will be unable to login. So I think it safer to keep it as it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess @nvtkaszpir is more or less about touching the point we have been struggling with lately. Regardless how many "must fail" scenarios we want to maintain, we didn't find a sane way to cleanly test a "must fail scenario" within the molecule test framework. I mean, how to write a test that's successful when Ansible complains in applying the role to a specific (incompatible) scenario with a given error message? It would have been nice if Ansible had a builtin mechanism to early fail based on some distro compatibility metadata, but I'm not aware of any. I'm also very concerned about long term maintenance of this runMolecule.sh
. As @macno pointed out, trying to forcefully apply the role to well known "must fail" scenarios could render the system unaccessible. Then we need to be extra safe here. Suggestions for a better approach here are hugely appreciated!
- add double quotes around URLs and other possible critical strings - remove empty lines between keys
Introduce a wrapper to invoke molecule.
Within the wrapper we are able to handle scenarios that correctly fail on some distro