Skip to content

make elasticsearch group name configurable #305

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

Conversation

zerwes
Copy link
Contributor

@zerwes zerwes commented Jan 22, 2024

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 ...

@martialblog martialblog requested a review from widhalmt January 25, 2024 10:19
Copy link
Member

@widhalmt widhalmt left a comment

Choose a reason for hiding this comment

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

Wow, thank you. That's a real nice addition. I had the same problem a few times but always got away with different inventories. Your approach will safe a lot of headache for a lot of people.

Only one real change I want to request. You couldn't know and we have to make it more prominent in the documentation: We're trying to stick to the new nomenclature for variables Ansible is using. Every variable defined in a role has to start with the name of the role. I know, there're some "elasticstack" variables but we will fix that in #250 .

The fix will bring yet another role "elasticstack" that will be imported by every other role. And it will mostly set default, variables and facts. Until this PR is merged, please rename your new variable to something like elasticstack_elasticsearch_group_name or elasticstack_es_group_name. For now you have to set it in every role like you already do. But we will move it to the new "global" role.

I guess, changing the name with grep, find and sed might work. Please let me know if I can help with that.

And please add your new variable to the list of variables, too. I really appreciate your change to the documentation but I hope to have the list as consistent as possible.

You offered to have the same change for all roles. This would actually be great and very much appreciated.

Thanks for your work so far.

@zerwes
Copy link
Contributor Author

zerwes commented Jan 27, 2024

please rename your new variable to something like elasticstack_elasticsearch_group_name

done in 7648992

@zerwes zerwes force-pushed the feature/make-elasticsearch-group-name-configurable branch from 17d8240 to 7648992 Compare January 27, 2024 10:48
@zerwes
Copy link
Contributor Author

zerwes commented Feb 4, 2024

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
Copy link
Member

widhalmt commented Feb 9, 2024

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.

@widhalmt widhalmt added this to the 0.1.0 milestone Feb 12, 2024
@zerwes
Copy link
Contributor Author

zerwes commented Feb 21, 2024

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 ....

Copy link
Member

@widhalmt widhalmt left a comment

Choose a reason for hiding this comment

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

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

@zerwes
Copy link
Contributor Author

zerwes commented Mar 16, 2024

@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
Copy link
Contributor Author

zerwes commented Apr 15, 2024

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

@widhalmt
Copy link
Member

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.

@widhalmt widhalmt merged commit ccd9052 into NETWAYS:main Apr 17, 2024
@zerwes zerwes deleted the feature/make-elasticsearch-group-name-configurable branch April 8, 2025 20:56
ivareri pushed a commit to ivareri/ansible-collection-elasticstack that referenced this pull request Jun 17, 2025
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 ...

---------

Co-authored-by: Klaus Zerwes <[email protected]>
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