Mepts 184 setup internal server for partners databases #4
Conversation
…chine configuration
…os_families implemeted
There was a problem hiding this comment.
@cfaife good first stab at ansible!
Left some comments of areas we could improve this and trim down to just what we need while also trying to reuse what we can. Take a look the comments and let know
@psmatsinhe please also take a look and leave your thoughts
| @@ -0,0 +1,490 @@ | |||
| # config file for ansible -- https://ansible.com/ | |||
There was a problem hiding this comment.
do we need these files under setup_internal_servers folder when we have the roles under the infrastructure folder?
If they are not being used can we remove them?
| allowed_ports: | ||
| - 80 | ||
| - 22 | ||
| - 8080 |
There was a problem hiding this comment.
Why are we specifying ports 8080 here? Port 22 is also defined in the code that enables ufw (look for OpenSSH)
Also looking at the 'basic' role, seems that ufw is not enabled...
| @@ -0,0 +1,38 @@ | |||
| Role Name | |||
There was a problem hiding this comment.
Is it needed to create a new 'basic' role, instead of modifying the existing 'common' role?
| @@ -0,0 +1,53 @@ | |||
| galaxy_info: | |||
There was a problem hiding this comment.
I am not familiar with galaxy_info - what is it meant for?
| @@ -0,0 +1,10 @@ | |||
| --- | |||
| #- name: enable ufw | |||
There was a problem hiding this comment.
How come not enabling ufw?
| @@ -0,0 +1,2 @@ | |||
| --- | |||
There was a problem hiding this comment.
please remove if not being used, Ansible does not need all these files to be able to run. We can add them when we need them - helps keep the code tidy
| @@ -0,0 +1,38 @@ | |||
| Role Name | |||
There was a problem hiding this comment.
Good idea to separate docker from the basic setup steps, but it would be better to refactor the 'common' role and then update the other playbooks to use the new docker role as well.
Just be sure to pay attention to the order of the ansible roles in the playbook to make sure docker gets run right after 'common'
| Example Playbook | ||
| ---------------- | ||
|
|
||
| Including an example of how to use your role (for instance, with variables passed in as parameters) is always nice for users too: |
There was a problem hiding this comment.
It is a good idea to include a readme with each role, however please try make it more usable and descriptive for that particular role. This current version, is very generic and looks like something I could copy paste and apply to all roles, which isn't too helpful for someone new
|
|
||
|
|
||
|
|
||
| - include: debian_based/install_docker.yml |
There was a problem hiding this comment.
can we maybe try reduce the amount of white space across our files?
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| - name: restart docker | |||
There was a problem hiding this comment.
what is the difference between reload and restart docker handlers?
No description provided.