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

Improved tasks and tests layout #25

Merged
merged 4 commits into from
Aug 20, 2018
Merged

Improved tasks and tests layout #25

merged 4 commits into from
Aug 20, 2018

Conversation

atosatto
Copy link
Owner

This PR:

  • makes sure that the tests are correctly executed by Travis
  • removes Ansible 2.3 from the CI test matrix
  • reorganize the tasks structure implementing a strict separation between the Minio server and Minio client installation tasks

@@ -12,3 +10,5 @@
- "/srv/data2"
- "/srv/data3"
- "/srv/data4"
roles:
Copy link
Collaborator

@paulfantom paulfantom Aug 20, 2018

Choose a reason for hiding this comment

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

Why swap order of roles and vars? This is a dict/map so from program point of view it doesn't change anything so we can only take an ease of code reading in account. Considering that roles key won't increase in size, but vars can, it would be better to have roles before vars as this won't cause problems in reading complex vars entries and simplify addition of new entries to this file.

Copy link
Owner Author

@atosatto atosatto Aug 20, 2018

Choose a reason for hiding this comment

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

The reason is that usually the vars refers to the declared roles. I think visually it's better to have the vars declared before the roles.
But I swapped them back in 2de48ef

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it is usually a matter of personal preference and usually with such things I look what guys from OpenStack-Ansible use since they have more written guidelines than ansible itself. In this case they use vars after roles, like here: https://github.com/openstack/openstack-ansible/blob/master/playbooks/infra-journal-remote.yml

Copy link
Owner Author

Choose a reason for hiding this comment

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

Indeed :)

'minio_server_bin',
'minio_client_bin',
])
def test_minio_installed(host, AnsibleDefaults, minio_bin_var):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not put this in test_minio_common.py?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Because otherwise that would be tested in every scenario.

The idea is to test it once, in the default scenario, and eventually add the proper tests if we decide to override the default values for the minio_server_bin and minio_client_bin variables.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not very fond of this solution, but it is ok 👍

@atosatto atosatto merged commit 00fdf97 into master Aug 20, 2018
@atosatto atosatto deleted the tasks-and-ci-cleanup branch August 20, 2018 18:25
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.

2 participants