Travis CI: Verify that "Ansible --check mode" passes#220
Travis CI: Verify that "Ansible --check mode" passes#220gildegoma wants to merge 2 commits intorvm:masterfrom
Conversation
|
Does not seem to help. The build is still failing :( |
|
The CI job detected a new problem, occurring only with the root.yml playbook. See https://travis-ci.org/github/rvm/rvm1-ansible/jobs/714023964#L296 for instance. |
| - docker-compose config --services | grep "$PLATFORM" | xargs docker-compose up -d | ||
| - ansible-playbook $PLAYBOOK -l "$PLATFORM:&latest" --check --diff | ||
| - ansible-playbook $PLAYBOOK -l "${PLATFORM:-latest}" -vv | ||
| - ansible-playbook $PLAYBOOK -l "$PLATFORM:&latest" --check --diff |
There was a problem hiding this comment.
Running before and after the effective ansible-playbook run verifies the check mode in two completely different situations. In the current root.yml failure (See "Symlink ruby related binaries on the system path" task), the problem only occurs when running --check before the role is applied.
It is to project maintainers to decide if they want that --check mode should work on an "empty" or "incompletely configured" system.
There was a problem hiding this comment.
Ok I see now. Can we add a comment to explain this?
I think we definitely want --check mode to pass, before we apply changes, as I think this is the more common workflow, at least for me and from what I understand. And I think it seems redundant to run again after normal mode, although I'm not entirely sure either way.
There was a problem hiding this comment.
I should add: I see what you mean about running --check mode a second time because the system is in a different state, but I think it's up to the normal mode test run to make sure any changes are good at this point.
There was a problem hiding this comment.
My second "--check mode" run is intended to guarantee that you can execute the role in check-mode without error.
But I get you're point, and actually it made me realize that there is something more important to verify: role idempotency, by executing twice the tasks (not in check mode). I'll put some more propositions... stay tuned 😉
There was a problem hiding this comment.
@danochoa please, look at c999c43. Without this change, the second part (rubies.yml) of the role would fail.
@pkuczynski @thbar I defer to you to decide if the 'check mode' should pass on a "non-bootstrapped" system (i.e. RVM not available yet).
Note: Proposal for the "full idempotency" check comes...
There was a problem hiding this comment.
Would you mind adding a comment in the code here too to explain running check mode twice? I think that would be helpful for someone looking at the code for the first time without reading our PR comments.
gildegoma
force-pushed
the
ci-check-mode
branch
from
181c558 to
bb22033
Compare
thbar
left a comment
thbar
left a comment
There was a problem hiding this comment.
Per #219 (comment), installing rubies all the time is very slow in my testing environment. I am not sure why the same code is on both PR, but the same reasoning applies!
For sake of CI speed, the check mode is verified only against one container (the latest version of each operating system platform). Such check is performed before, and after, the Ansible role has been applied to the managed instance.
gildegoma
force-pushed
the
ci-check-mode
branch
from
bb22033 to
0a1aa09
Compare
Skip all the 'rubies.yml' tasks if RVM is not installed yet.
|
Rebased and now ready for review. |
danochoa
left a comment
danochoa
left a comment
There was a problem hiding this comment.
I think some comments could help explain a couple things that might seem unusual but feel free to ignore. Otherwise I think this looks good.
| become: yes | ||
| become_user: '{{ rvm1_user }}' | ||
|
|
||
| - name: Check that RVM is installed |
There was a problem hiding this comment.
I'm not too sure about this but I think its ok to let the role fail if nothing has been installed, rather than adding tasks like this to satisfy check mode. Do you mind adding a comment in the code here to explain this task to help avoid confusion (since the previous tasks are there to ensure RVM is installed)?
There was a problem hiding this comment.
I agree with you. For the moment, I made this to be "always" check mode compatible, but I am not sure if it is worth it. Up to core maintainers to decide...
| command: '{{ rvm1_rvm }} list strings' | ||
| register: rvm_list_strings | ||
| changed_when: False | ||
| check_mode: no # Run in normal mode when in --check mode (http://docs.ansible.com/ansible/playbooks_checkmode.html) |
| - docker-compose config --services | grep "$PLATFORM" | xargs docker-compose up -d | ||
| - ansible-playbook $PLAYBOOK -l "$PLATFORM:&latest" --check --diff | ||
| - ansible-playbook $PLAYBOOK -l "${PLATFORM:-latest}" -vv | ||
| - ansible-playbook $PLAYBOOK -l "$PLATFORM:&latest" --check --diff |
There was a problem hiding this comment.
Would you mind adding a comment in the code here too to explain running check mode twice? I think that would be helpful for someone looking at the code for the first time without reading our PR comments.
@danochoa thank you for all the feebacks 😃. Yes, if we stick the goal to support "check mode" in any situation, I'll add a comment to try to make things crystal clear. |
|
As we are moving away from Travis to GHA, is this still valid @danochoa ? |
|
See #235, which maybe will have to be merged first, to see if current PR is worth adapting to GH actions. |
|
@gildegoma as we migrated to Github Actions in #235, is this still somehow valid, and would you like to migrate this to the new CI? |
|
Related: #226 |
|
Ping @gildegoma... |
4 participants
Initially motivated by #218.
In order to get a build success, this PR (temporarily) includes the changes proposed in #219.