Skip to content

Conversation

widhalmt
Copy link
Member

fixes #159

@widhalmt widhalmt added the feature New feature or request label May 16, 2023
@widhalmt widhalmt added this to the 1.0.0 milestone May 16, 2023
@widhalmt widhalmt self-assigned this May 16, 2023
@widhalmt widhalmt marked this pull request as draft May 16, 2023 11:42
@widhalmt
Copy link
Member Author

I made this a draft because there are still some variables missing in the renaming. We'll discuss how to proceed with them in #159

widhalmt added 2 commits May 16, 2023 13:54
We opened a discussion around that issue upstream: ansible/ansible-lint#3451

To move fast, we now put this on the warn_list and come back to it, when
the discussion is resolved. Hopefully to remove the rule completly from
`ansible-lint.yml`.
@widhalmt widhalmt marked this pull request as ready for review May 16, 2023 12:46
@widhalmt
Copy link
Member Author

We opened a discussion around that issue upstream: ansible/ansible-lint#3451

To move fast, we now put this on the warn_list and come back to it, when
the discussion is resolved. Hopefully to remove the rule completly from
ansible-lint.yml.

@widhalmt widhalmt enabled auto-merge May 16, 2023 12:47
@widhalmt widhalmt requested a review from lcndsmr May 16, 2023 12:49
widhalmt added a commit that referenced this pull request May 16, 2023
Copy link
Member

@dgoetz dgoetz left a comment

Choose a reason for hiding this comment

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

If the collection is accepted as prefix in the upstream discussion everything related to the CA shared by the complete stack should be prefixed elasticstack_, this goes for unchanged variables like elastic_ca_pass and some changed in the PR like elastic_ca_will_expire_soon/elasticsearch_ca_will_expire_soon. So I would keep thise unchanged until then.

@widhalmt
Copy link
Member Author

@dgoetz I hope, I now caught all of the missing variables. Honestly, I feel a slight hint of panic when I think of merging this PR and all the others that are still open. 😱

@widhalmt
Copy link
Member Author

Some of the checks are failing because of timeouts. As long as all lint checks and the required checks work, we should be safe.

@afeefghannam89
Copy link
Member

afeefghannam89 commented May 17, 2023

@widhalmt woo It is a big type work ✍️ Thanks I see the PR is ok. The only suggestion is the above, there are three stack words in the variable name. You can take the suggestion or ignore it.

widhalmt and others added 3 commits May 17, 2023 12:34
Rename variable for better readability.

Co-authored-by: Afeef Ghannam <39904920+afeefghannam89@users.noreply.github.com>
Copy link
Member

@dgoetz dgoetz left a comment

Choose a reason for hiding this comment

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

Naming looks consistent now within the collection and with the guideline (including our proposed addition). It creates some clunky names but that seems unavoidable with the goal in mind. Nitpicking could be done in form of complaining some not being in alphabetical or logical order, but I am ok with this.

@widhalmt
Copy link
Member Author

Some of them aren't in alphabetical order on purpose. I tried to group them logically. :-)

@widhalmt widhalmt added this pull request to the merge queue May 17, 2023
Merged via the queue into main with commit 07f5b40 May 17, 2023
@widhalmt widhalmt deleted the fix/variable-naming branch May 17, 2023 12:49
ivareri pushed a commit to ivareri/ansible-collection-elasticstack that referenced this pull request Jun 17, 2025
fixes NETWAYS#159

---------

Co-authored-by: Afeef Ghannam <39904920+afeefghannam89@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Variable naming conventions

3 participants