Skip to content
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

Fix breaks when mongodb subchart is not used #291

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bingwei-hong-partior
Copy link
Contributor

What this PR does / why we need it:

This is to fix issues when external mongoDB is used (.Values.mongodb.enabled is set as false), result in rendering errors in the few templates.
a few templates.

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • fixes #

Special notes for your reviewer:

Have tested both following

  1. independent deployment of mongoDB then deploy the litmus helmcharts
  2. deployment of litmus with mongoDB subcharts

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

Signed-off-by: bingwei-hong-partior <[email protected]>
@Jasstkn
Copy link
Member

Jasstkn commented Nov 26, 2022

@bingwei-hong-partior could you provide an example of values and errors - it will help to gather more context around the issue.

@bingwei-hong-partior
Copy link
Contributor Author

bingwei-hong-partior commented Nov 26, 2022

when mongodb.enabled is set as true, chart can be rendered properly
image

when mongodb.enabled is set as false, _helpers.tpl will fail to render properly due to subcharts no longer included. This result in unable to use external mongodb instance.

subchart dependency have a condition
image

when mongodb.enabled set as false in values.yaml file, mongodb subchart will not be available, result in lint failure as shown below
image

lint shows rendering error
image

both server-deployment.yaml and auth-server-deployment.yaml also using .Values.mongodb.service.ports.mongodb without checking if mongoDB is enabled.
image
instead of using these from the adminConfig as specified in the values.yaml file.
image

@Jasstkn , please let me know if there is a better way of fixing this without doing these changes.

May I also ask if there is a way to cater for allow specifying the dependency's repository url? This is to cater for situations where we prefer to pull all the helm repo via our jfrog remote as proxy. In current setup, dependency repo url is hard-coded.

Signed-off-by: bingwei-hong-partior <[email protected]>
@bingwei-hong-partior
Copy link
Contributor Author

Hi Team, could you review and let me know if any concern on this PR. thank you.

@bingwei-hong-partior
Copy link
Contributor Author

@imrajdas , @Jasstkn , @ispeakc0de , could you please review this PR. Let me know if additional information is required. thank you.

@Jasstkn
Copy link
Member

Jasstkn commented Dec 8, 2022

@imrajdas , @Jasstkn , @ispeakc0de , could you please review this PR. Let me know if additional information is required. thank you.

Hi! Sorry for the delayed reply - I'll take a closer look later today.

@bingwei-hong-partior
Copy link
Contributor Author

@imrajdas , @Jasstkn , @ispeakc0de , could you please review this PR. Let me know if additional information is required. thank you.

Hi! Sorry for the delayed reply - I'll take a closer look later today.

@Jasstkn , any updates regarding this PR? I am actually needing this PR, otherwise I have to maintain my own fork which is not really something I want to do.

Signed-off-by: Maria Kotlyarevskaya <[email protected]>
Copy link
Member

@Jasstkn Jasstkn left a comment

Choose a reason for hiding this comment

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

Hi. I'm very sorry for this incredibly long review.

I took some time to inspect the issue and the solution and proposed some changes. If you are willing to make the suggested improvements - feel free to do so.

@@ -42,7 +42,11 @@ spec:
command: ["/bin/sh", "-c"]
args:
[
{{- if .Values.mongodb.enabled }}
"while [[ $(curl -sw '%{http_code}' http://{{ include "litmus-portal.mongodbServiceName" . }}:{{ .Values.mongodb.service.ports.mongodb }} -o /dev/null) -ne 200 ]]; do sleep 5; echo 'Waiting for the MongoDB to be ready...'; done; echo 'Connection with MongoDB established'",
Copy link
Member

Choose a reason for hiding this comment

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

Can we calculate port the same way as we do with name? just move this logic to template and include the result.

"while [[ $(curl -sw '%{http_code}' http://{{ include "litmus-portal.mongodbServiceName" . }}:{{ .Values.mongodb.service.ports.mongodb }} -o /dev/null) -ne 200 ]]; do sleep 5; echo 'Waiting for the MongoDB to be ready...'; done; echo 'Connection with MongoDB established'",
{{- else }}
"while [[ $(curl -sw '%{http_code}' http://{{ include "litmus-portal.mongodbServiceName" . }}:{{ .Values.adminConfig.DB_PORT }} -o /dev/null) -ne 200 ]]; do sleep 5; echo 'Waiting for the MongoDB to be ready...'; done; echo 'Connection with MongoDB established'",
Copy link
Member

Choose a reason for hiding this comment

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

the same goes here.

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