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.

Sign up for GitHub

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

Travis CI: Verify that "Ansible --check mode" passes #220

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

Travis CI: Verify that "Ansible --check mode" passes #220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a1aa09
Travis CI: Verify that Ansible check mode passes
gildegoma Aug 1, 2020
c999c43
Fix Check Mode
gildegoma Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ install:

script:
- cd tests
- docker-compose config --services | grep $PLATFORM | xargs docker-compose up -d
- ansible-playbook $PLAYBOOK -l $PLATFORM
- docker-compose config --services | grep "$PLATFORM" | xargs docker-compose up -d
- ansible-playbook $PLAYBOOK -l "$PLATFORM:&latest" --check --diff
- ansible-playbook $PLAYBOOK -l "$PLATFORM" -vv
- ansible-playbook $PLAYBOOK -l "$PLATFORM:&latest" --check --diff
Copy link
Contributor

Choose a reason for hiding this comment

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

why run check mode twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@gildegoma gildegoma Aug 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


before_cache:
# Put docker images used during this run into travis cache (if not already cached).
Expand Down
9 changes: 9 additions & 0 deletions tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,16 @@
become: yes
become_user: '{{ rvm1_user }}'

- name: Check that RVM is installed
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

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 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...

stat:
path: '{{ rvm1_rvm }}'
register: rvm_binary_check
ignore_errors: ansible_check_mode
changed_when: no
check_mode: no # Run in normal mode when in --check mode (http://docs.ansible.com/ansible/playbooks_checkmode.html)

- name: Install Ruby and Gems
import_tasks: 'rubies.yml'
become: yes
become_user: '{{ rvm1_user }}'
when: rvm_binary_check.stat.exists
1 change: 1 addition & 0 deletions tasks/rubies.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


- name: Install rubies
command: '{{ rvm1_rvm }} install {{ item }} {{ rvm1_ruby_install_flags }}'
gildegoma marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
5 changes: 5 additions & 0 deletions tests/inventory
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,8 @@ tests_ubuntu14_1 ansible_connection=docker
tests_ubuntu16_1 ansible_connection=docker
tests_ubuntu18_1 ansible_connection=docker
tests_ubuntu20_1 ansible_connection=docker

[latest]
tests_centos8_1
tests_debian10_1
tests_ubuntu20_1
1 change: 1 addition & 0 deletions tests/root.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@
rvm1_install_path: '/opt/rvm'
tasks:
- import_tasks: assertions.yml
when: not ansible_check_mode
tags: validate
1 change: 1 addition & 0 deletions tests/user.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@
rvm1_install_path: '/home/user/.rvm'
tasks:
- import_tasks: assertions.yml
when: not ansible_check_mode
tags: validate