-
Notifications
You must be signed in to change notification settings - Fork 10
Initial commit for Ansible based Satellite6 Install and Configure. #4
Conversation
kbidarkar
commented
Jul 18, 2018
- Added a partition-disk role
- Added playbooks for sat63 and sat64
ae88569
to
3bf95bc
Compare
* Added a partition-disk role * Added playbooks for sat63 and sat64 Signed-off-by: Kedar Bidarkar <[email protected]>
3bf95bc
to
a9d5317
Compare
[First run for the host: sat64-rhel7] Below are the results for the partition-disk role while using the sat64 playbook.
[Re-run for the host sat63-rhel7]Also, tried re-running the partition-disk role using sat63 playbook.
Feel, it is better to also test re-run, just to avoid any issues while re-running the playbook on the same host. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left 1 comment
``` | ||
git clone [email protected]:yourname/ansible-satellite6 | ||
cd ansible-satellite6 | ||
git checkout -b your-branch-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have them add the remote so they can rebase easily and pull in new changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That suggestion is based on this link, https://help.github.com/articles/fork-a-repo/ :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will mark my comment approved then :)
Was talking to @kbidarkar on IRC about the need to make sure that the underlying version of RHEL does not affect the outcome of this task. Like setting up the firewall rules on RHEL 6 x RHEL 7 :) |
CONTRIBUTING.md
Outdated
========= | ||
Install and Run [ansible-lint](https://github.com/willthames/ansible-lint) against your code changes. | ||
|
||
Also, Install and Run [ansible-review](https://github.com/willthames/ansible-review) against your code changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to consider conforming to ansible-galaxy init --offline when creating roles as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, will be sending out a mail about all the best-practices stuff soon.
Guys, nothing is fixed in stone, this initial commit is to get started, we can evolve the project depending upon the needs moving forward. |
README.md
Outdated
|
||
This repository will be Python3.6 based. | ||
|
||
You need Ansible installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a minimum Ansible version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I am testing/working with 2.6, any suggestions as to which versions should we be supporting from? Currently, I think of from 2.5+ . Thoughts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should just test with Ansible 2.5 too and set that as the min version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still pending, will test and report here and then a re-review. Post which we will merge it.
ansible.cfg
Outdated
module_utils = foreman-ansible-modules/module_utils | ||
roles_path = $PWD/galaxy_roles:$PWD/roles | ||
inventory = inventory | ||
retry_files_enabled = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few others to consider:
force_color = 1
stdout_callback = yaml
display_skip_hosts = false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will take a look at each of those options and will add it up.
ansible.cfg
Outdated
library = foreman-ansible-modules/modules | ||
module_utils = foreman-ansible-modules/module_utils | ||
roles_path = $PWD/galaxy_roles:$PWD/roles | ||
inventory = inventory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend making an inventories/ directory and putting the sample in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it too, even @flatfender was suggesting the same, I just followed this from Ansible and so kept it at top-level:
https://docs.ansible.com/ansible/latest/user_guide/playbooks_best_practices.html#directory-layout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will make this as inventory_files/inventory
, also would move inventory.sample
file to inventory_files
, else suggest me a name :)
@@ -0,0 +1,2 @@ | |||
--- | |||
# handlers file for partition-disk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these files are gonn abe empty, I'd just delete them to reduce the noise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the Best-practices etherpad which I created and shared with @flatfender, I added this point to delete unused files created after we run ansible-galaxy init
, but @flatfender mentioned we maintain the dir_structure as it is.
I think the idea of @flatfender was to have these as it is, for the first/initial role, so that it be an example/reference for the directory structure we want to put in place.
Maybe moving forward we can remove it, ok will discuss with @flatfender about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I tend to think the docs on roles should suffice and the code repository should have as little noise as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the idea was consistency around ansible-galaxy init, so that all roles have consistent structure and empty dirs don't hurt, and it makes it more galaxy friendly if we then commit back to galaxy. Once you delete them, then people tend to make things up later and we get inconsistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think given that Ansible roles have a defined structure there is no worry about things getting made up. But I do feel "noise" is a valid concern. I dislike going into code bases finding empty useless files and wasting my time on them personally. That makes debugging harder IMO.
roles/partition-disk/meta/main.yml
Outdated
# - GPLv2 | ||
# - GPLv3 | ||
# - Apache | ||
# - CC-BY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could get rid of all these comments in here.
roles/partition-disk/meta/main.yml
Outdated
galaxy_info: | ||
author: Satellite QE Team | ||
description: Satellite QE Team | ||
company: RedHat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Red Hat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh! I knew about this, my bad will fix it.
fstype: xfs | ||
resizefs: yes | ||
with_dict: "{{ ansible_lvm.lvs }}" | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the three dots here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use that for YAML files to mark end of the file, as I had read about it here, https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html#yaml-basics
@@ -0,0 +1,16 @@ | |||
- name: "Extend root partition LV to all available VG space" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be indented at column 0
state: absent | ||
force: yes | ||
with_dict: "{{ ansible_lvm.lvs }}" | ||
... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these can be dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use that for YAML files to mark end of the file, as I had read about it here, https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html#yaml-basics :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned something new today. I have never seen that before so I might recommend not including them to reduce confusion. I do a see a mixed use of ---
at the start of YAML across all YAML I have run across.
mount: | ||
name: /home | ||
state: absent | ||
- name: "Remove /home Logical Volume" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it helps readability to separate tasks by an empty line just like you would a function definition in python code
c9908f1
to
ed6c75e
Compare
68634b8
to
7d58bcc
Compare
*) Fixed as per yamllint *) Fixed as per ansible-review and ansible-lint *) Moved inventory file to inventory_files *) Added standards.py under rules/ansible-review. *) Updated ansible.cfg as per suggestions *) Added some best practices to CONTRIBUTING.md Signed-off-by: Kedar Bidarkar <[email protected]>
7d58bcc
to
7418646
Compare