NETWAYS / ansible-collection-elasticstack

A collection to install and manage the Elastic Stack
GNU General Public License v3.0
10 stars 8 forks source link

make elasticsearch group name configurable #305

Closed zerwes closed 5 months ago

zerwes commented 8 months ago

For my setup I need multiple single-node instances, and here I get into trouble with the hard coded group name. This fixes this limitation for the elasticsearch role only. If it is desired, I can implemented it for all roles ...

zerwes commented 8 months ago

please rename your new variable to something like elasticstack_elasticsearch_group_name

done in 7648992

zerwes commented 8 months ago

while renaming the remaining hard coded group names, i stumbled over elasticsearch_role_master ...

~/git/ansible-collection-elasticstack feature/make-all-group-names-configurable* ± git grep "groups\['elasticsearch_role_master']"
roles/elasticsearch/tasks/main.yml:        count_of_master_nodes: "{{ groups['elasticsearch_role_master'] | length }}"
roles/elasticsearch/templates/elasticsearch.yml.j2:cluster.initial_master_nodes: [ {% for host in groups['elasticsearch_role_master']  %}

I did not dive deeper into this, but I think this should be configurable too, am I right?

widhalmt commented 7 months ago

while renaming the remaining hard coded group names, i stumbled over elasticsearch_role_master ...

~/git/ansible-collection-elasticstack feature/make-all-group-names-configurable* ± git grep "groups\['elasticsearch_role_master']"
roles/elasticsearch/tasks/main.yml:        count_of_master_nodes: "{{ groups['elasticsearch_role_master'] | length }}"
roles/elasticsearch/templates/elasticsearch.yml.j2:cluster.initial_master_nodes: [ {% for host in groups['elasticsearch_role_master']  %}

I did not dive deeper into this, but I think this should be configurable too, am I right?

Yes, you're totally right. I never liked these hardcoded roles. I guess, I just had a hard time getting rid of them once I started. I'm really happy and thankful you got this, now.

zerwes commented 7 months ago

I did not dive deeper into this, but I think this should be configurable too, am I right?

Yes, you're totally right. I never liked these hardcoded roles. I guess, I just had a hard time getting rid of them once I started. I'm really happy and thankful you got this, now.

for now I leave elasticsearch_role_master untouched, has just 2 occurrence ....

zerwes commented 6 months ago

@zerwes Everything approved. But the PR now has merge conflicts. Can you fix them? If you want help, please just let me know.

@widhalmt : merge conflicts resolved in 29c802b for my part, the checks are green and OK on the fork ...

zerwes commented 5 months ago

Hello @widhalmt Is there anything I can do here? Greetings

widhalmt commented 5 months ago

Hi @zerwes , not really. I was hoping the checks would start eventually. I'll see into it that you finally get your PR merged. It's quite useful nonetheless. Thank you again for all the work and sorry for taking so long.