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

Nida #2

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Nida #2

wants to merge 7 commits into from

Conversation

nidakhan990
Copy link
Collaborator

changes added

Comment on lines +4 to +5
filters:
"tag:Name": "loki"

Choose a reason for hiding this comment

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

Also add tags for promtails so that we can install promtail agent on all VMs where every required

Choose a reason for hiding this comment

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

also add keygroups in dynamic inventory and make sure we make use of private IP to list the instance using dynamic inventory.

[defaults]
inventory = aws_ec2.yml # Path to your inventory file
host_key_checking = False # Optional: disable host key checking for SSH
remote_user = ubuntu # Default user for SSH connections

Choose a reason for hiding this comment

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

No need to degine the remote_user as ubuntu as we are going to use the same role to redhat and centos as well. So default user name will change

Choose a reason for hiding this comment

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

update the README.md

Choose a reason for hiding this comment

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

if meta.yml is not required then delete it

Comment on lines +2 to +8
- name: Install prerequisite packages on Debian-based systems
ansible.builtin.apt:
name:
- gnupg
- curl
state: present
when: ansible_facts['os_family'] == 'Debian'

Choose a reason for hiding this comment

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

As we discussed earlier as well variables the package name don't hardcode it inside the role


- name: Add Grafana GPG key on Debian-based systems
ansible.builtin.apt_key:
url: https://packages.grafana.com/gpg.key

Choose a reason for hiding this comment

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

this url should be also be variablised


- name: Add Grafana repository on Debian-based systems
ansible.builtin.apt_repository:
repo: "deb https://packages.grafana.com/oss/deb stable main"

Choose a reason for hiding this comment

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

This also need to variables

state: present
when: ansible_facts['os_family'] == 'Debian'

- name: Update apt package repository on Debian-based systems

Choose a reason for hiding this comment

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

can't we do update cache and install grafana in single. Command I think it is achievable please look into this

Comment on lines +34 to +40
- name: Install prerequisite packages on Red Hat-based systems
ansible.builtin.yum:
name:
- gnupg
- curl
state: present
when: ansible_facts['os_family'] == 'RedHat'

Choose a reason for hiding this comment

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

Variables the package name inside the vars.yml shouldn't be hardcoded

- name: Add Grafana GPG key on Red Hat-based systems
ansible.builtin.rpm_key:
state: present
key: https://packages.grafana.com/gpg.key

Choose a reason for hiding this comment

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

make this as a variable

Comment on lines +48 to +63
- name: Add Grafana repository on Red Hat-based systems
ansible.builtin.yum_repository:
name: grafana
description: Grafana OSS Repository
baseurl: https://packages.grafana.com/oss/rpm
gpgcheck: 1
gpgkey: https://packages.grafana.com/gpg.key
enabled: 1
when: ansible_facts['os_family'] == 'RedHat'

- name: Install Grafana on Red Hat-based systems
ansible.builtin.yum:
name: grafana
state: present
when: ansible_facts['os_family'] == 'RedHat'
notify: starting grafana

Choose a reason for hiding this comment

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

variabilize the package name nothing should be hard coded inside the tasks.

Comment on lines +6 to +13
- name: Install unzip, bzip2, xz-utils, and zstd
ansible.builtin.apt:
name:
- unzip
- bzip2
- xz-utils
- zstd
state: present

Choose a reason for hiding this comment

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

same here not variabilized


- name: Add the user 'loki'
ansible.builtin.user:
name: loki

Choose a reason for hiding this comment

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

username should also be variabilized

Comment on lines +44 to +54
- name: Creating directory for config file
ansible.builtin.file:
path: "{{ item }}"
state: directory
owner: loki
group: loki
mode: '755'
recurse: true
loop:
- /etc/loki/logs
- /data/loki

Choose a reason for hiding this comment

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

this directory path should also be define in vars.yml

Comment on lines +56 to +59
- name: Creating Config file
ansible.builtin.file:
path: /etc/loki/loki-local-config.yaml
state: touch

Choose a reason for hiding this comment

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

This file should also be not created as we are going to copy it from our ansible controller to slave. So doesn't requires to create it

Comment on lines +62 to +65
- name: Copying the config file content
ansible.builtin.template:
src: templates/loki-config.j2
dest: /etc/loki/loki-local-config.yaml

Choose a reason for hiding this comment

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

Have we tested this if it is working fine or not. I think you haven't add values to jinja template till now directly copy pasting the jinja template.

Comment on lines +67 to +70
- name: Creating service file
ansible.builtin.file:
path: /etc/systemd/system/loki.service
state: touch

Choose a reason for hiding this comment

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

Same here not required to create a file

- name: Include grafana tasks
ansible.builtin.include_tasks:
file: grafana.yaml
when: run_grafana_tasks

Choose a reason for hiding this comment

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

change it to something like
install_grafana == true

- name: Include Loki tasks
ansible.builtin.include_tasks:
file: loki.yaml
when: run_loki_tasks

Choose a reason for hiding this comment

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

Same here refer to above comment

- name: Include promtail tasks
ansible.builtin.include_tasks:
file: promtail.yml
when: run_promtail_tasks

Choose a reason for hiding this comment

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

Same here

Comment on lines +3 to +8
- name: Install prerequisite packages
package:
name:
- unzip
state: present
when: ansible_facts['os_family'] in ['Debian', 'RedHat']

Choose a reason for hiding this comment

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

variabilize the package name

Comment on lines +21 to +23
- name: Move Promtail binary to /usr/local/bin
ansible.builtin.command:
cmd: mv /usr/local/bin/promtail-linux-amd64 /usr/local/bin/promtail

Choose a reason for hiding this comment

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

can't we do it using ansible builtin module

Comment on lines +47 to +50
- name: Reload systemd daemon
ansible.builtin.systemd:
daemon_reload: true

Choose a reason for hiding this comment

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

move it to handler section

Comment on lines +54 to +59
- name: Check Promtail service status
ansible.builtin.command:
cmd: systemctl status promtail.service
register: promtail_status
changed_when: false

Choose a reason for hiding this comment

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

can't we ansible builtin module

Choose a reason for hiding this comment

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

insight of this make use of molecule test cases to test ansible role

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants