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

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

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

Conversation

gildegoma
Copy link
Contributor

Initially motivated by #218.

In order to get a build success, this PR (temporarily) includes the changes proposed in #219.

@gildegoma gildegoma requested a review from a team as a code owner August 1, 2020 15:43
@gildegoma gildegoma requested a review from danochoa August 1, 2020 15:44
@pkuczynski
Copy link
Member

Does not seem to help. The build is still failing :(

@gildegoma
Copy link
Contributor Author

gildegoma commented Aug 1, 2020

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

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.

.travis.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@thbar thbar left a comment

Choose a reason for hiding this comment

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

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!

tasks/rubies.yml Show resolved Hide resolved
@gildegoma gildegoma marked this pull request as draft August 5, 2020 09:32
@thbar
Copy link
Contributor

thbar commented Aug 18, 2020

We have updated & tested #219. I believe it will be good to first merge #219, then update #220!

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.
Skip all the 'rubies.yml' tasks if RVM is not installed yet.
@gildegoma
Copy link
Contributor Author

Rebased and now ready for review.

@gildegoma gildegoma marked this pull request as ready for review August 19, 2020 04:50
Copy link
Contributor

@danochoa danochoa left a comment

Choose a reason for hiding this comment

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

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.

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

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

👍

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

@gildegoma
Copy link
Contributor Author

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.

@pkuczynski
Copy link
Member

pkuczynski commented Mar 25, 2022

As we are moving away from Travis to GHA, is this still valid @danochoa ?

@thbar
Copy link
Contributor

thbar commented Mar 26, 2022

See #235, which maybe will have to be merged first, to see if current PR is worth adapting to GH actions.

@pkuczynski
Copy link
Member

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

@thbar
Copy link
Contributor

thbar commented Mar 30, 2022

Related: #226

@pkuczynski
Copy link
Member

Ping @gildegoma...

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