Skip to content

Conversation

@xtruthx
Copy link
Contributor

@xtruthx xtruthx commented Aug 9, 2023

This PR fixes #219

@xtruthx xtruthx changed the title make elasticsearch network.host make elasticsearch network.host configurable Aug 9, 2023
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.

Thanks for the PR!

I'm just wondering, should we use a single string for the variable or should we make the variable an array since the option in the file is an array, too?

I'm ok with your string variant, especially since using Jinja2 for an array would be much more complicated (like ifnotlast, then "," etc)

So, approved. Thanks, again.

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.

Sorry, I had to change back to "Request changes". Could you please write a line about the new variable in the appropriate file in docs/?

@xtruthx
Copy link
Contributor Author

xtruthx commented Aug 11, 2023

Sorry, I had to change back to "Request changes". Could you please write a line about the new variable in the appropriate file in docs/?

I will avoid doiing duplicate work. This will be done within the PR for the documentation review. Pls accept the PR without additionl docs, anyway if you not set it the "legacy" default will take precedence.

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.

Ah, ok. That's fine, of course. So - approved.

@widhalmt widhalmt enabled auto-merge August 11, 2023 07:57
@xtruthx
Copy link
Contributor Author

xtruthx commented Aug 11, 2023

the fix of #217 conflicts :/

@lcndsmr
Copy link
Member

lcndsmr commented Aug 11, 2023

Its just a renaming of variables, you should just replace the names and it should work. I can do it quickly if you want to?

auto-merge was automatically disabled August 11, 2023 08:55

Head branch was pushed to by a user without write access

@xtruthx
Copy link
Contributor Author

xtruthx commented Aug 11, 2023

Its just a renaming of variables, you should just replace the names and it should work. I can do it quickly if you want to?

thx, its already done.

@widhalmt widhalmt enabled auto-merge August 11, 2023 10:06
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.

Sorry, I have to revert my approval. I thought a lot about how the collection is proceeding and I'm afraid we have to face the fact that we aren't full time developers andtherefore we can be sure that we run into times where changes will have to wait for a long time. (You had to experience that for yourself). So the only way to not lose focus and have all neccessary steps done right away is to make changes atomic. Therefore I opened an issue to force documentation with every step: #234 .

So please add documentation for that variable with this PR.

auto-merge was automatically disabled August 25, 2023 15:28

Head branch was pushed to by a user without write access

@xtruthx
Copy link
Contributor Author

xtruthx commented Aug 25, 2023

Sorry, I have to revert my approval. I thought a lot about how the collection is proceeding and I'm afraid we have to face the fact that we aren't full time developers andtherefore we can be sure that we run into times where changes will have to wait for a long time. (You had to experience that for yourself). So the only way to not lose focus and have all neccessary steps done right away is to make changes atomic. Therefore I opened an issue to force documentation with every step: #234 .

So please add documentation for that variable with this PR.

I've add a description to the documenation.

@xtruthx xtruthx requested a review from widhalmt August 25, 2023 15:36
@widhalmt widhalmt enabled auto-merge August 30, 2023 14:45
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.

Thank you! Approved.

@widhalmt widhalmt added this pull request to the merge queue Aug 30, 2023
Merged via the queue into NETWAYS:main with commit a65d969 Aug 30, 2023
ivareri pushed a commit to ivareri/ansible-collection-elasticstack that referenced this pull request Jun 17, 2025
This PR fixes  NETWAYS#219

---------

Co-authored-by: Thomas Widhalm <thomas.widhalm@netways.de>
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.

Posibility network.host as variable

3 participants