Fixing opensearch helmchart conditions when there is only cluster_manager role specified to nodes #380
Conversation
MindAwakeBodyAsleep
requested review from
a team,
TheAlgo and
DandyDeveloper
as code owners
…ager role specified to nodes Signed-off-by: Hubert Klasa <[email protected]>
MindAwakeBodyAsleep
force-pushed
the
bugfix/cluster_manager_specific_conditions_fix
branch
from
c8bb735 to
e6966d1
Compare
Signed-off-by: Hubert Klasa <[email protected]>
MindAwakeBodyAsleep
force-pushed
the
bugfix/cluster_manager_specific_conditions_fix
branch
from
e6966d1 to
aeb36b1
Compare
|
@TheAlgo @DandyDeveloper have you had a chance to take a look at that PR? :) |
|
@TheAlgo @DandyDeveloper @peterzhuamazon @prudhvigodithi Can you please review this PR? |
|
Hey @MindAwakeBodyAsleep thanks for the contribution, the condition added LGTM, but can you please share the issue you are facing ? |
| @@ -354,7 +354,7 @@ spec: | |||
| valueFrom: | |||
| fieldRef: | |||
| fieldPath: metadata.name | |||
| {{- if (and (has "master" .Values.roles) (not .Values.singleNode)) }} | |||
| {{- if (and (or (has "master" .Values.roles) (has "cluster_manager" .Values.roles)) (not .Values.singleNode)) }} | |||
There was a problem hiding this comment.
Good to add description in README.md as well for the same. Also echoing on the question by @prudhvigodithi how is it impacting and what are the failures we are seeing. Good to capture it in the github issue
There was a problem hiding this comment.
I can provide some input on the question posed by @prudhvigodithi . I recently was deploying Opensearch for the first time and was having issues when there was more than one replica. I had been attempting to use all inclusive language, to include specifying a new roles key in my values.yaml where I replaced the master role with cluster_manger. I tracked down the issue to these conditional renderings in the helm chart. I solved it by just using the non-inclusive language.
|
This seems only a change for 2.x+ as 1.x does not have cluster_manager term. |
|
Hey @MindAwakeBodyAsleep just following back, can you please take care of the previous comments and we can merge this PR. |
|
Hey @MindAwakeBodyAsleep just following back, can you please take care of the previous comments and we can merge this PR, also I do see some conflicts can you please fix them as well? |
There was a problem hiding this comment.
@MindAwakeBodyAsleep Checking for the comments on the PR. Thanks
|
@MindAwakeBodyAsleep Checking for the comments on the PR. Thanks |
|
Glad I received the email notification on this, I had completely forgotten about this and wanted to keep tabs on it. Since the original author didn't seem to follow through, I created a new PR that covers all inclusive language changes: #560 |
|
Thanks @Phenix66. |
Description
As new opensearch role for nodes was introduce helmchart templates should be adjusted. I Fixed opensearch helmchart conditions when there is no master role specified to nodes.
Issues Resolved
[List any issues this PR will resolve. You should likely open an issue if one does not already exist.]
Check List
For any changes to files within Helm chart directories:
CHANGELOG.mdupdated to reflect changeBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.