-
Notifications
You must be signed in to change notification settings - Fork 3
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
Remove --skip-refresh option from scripts/update-charts-dep.sh #30
base: master
Are you sure you want to change the base?
Conversation
Helm is failing to download dependencies from external chart repos with this option specified, so we need to remove it. One of such dependencies is reporting templates helm chart, which is included as a dependency for reporting service. Removing this option shouldn't affect anything, as it is probably used for optimization purposes only.
@@ -41,7 +41,7 @@ echo "Updating all Charts..." | |||
for chart in "${charts[@]}" | |||
do | |||
echo "---=== Updating $chart ===---" | |||
helm dep up "$chart" --skip-refresh | |||
helm dep up "$chart" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reverse.
One (or the CI/CD scripts) should run helm repo update
prior to running this, which is already done here by circleci --> https://github.com/mojaloop/charts/blob/master/.circleci/config.yml#L46
Refer to this --> https://helm.sh/docs/helm/helm_dependency_update/#options
The reason this was not included in this script, is because it is running a helm dep update
for EVERY single chart. It's unnecessary and very slow, thus why it should be run at the top level, and only once.
Note, this does mean that if there are any external dependencies (which there should NOT be), we would need to add those repo's manually (helm repo add <name> <URL>
) before running the helm repo update
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we are running helm dep update
for every dependency, instead of high-level charts, i.e. bof
, mojaloop, etc. Isn't it the job of Helm to do manage dependencies?
For me it looks like the same if we were parsing package.json
with a script and trying to install all dependencies manually instead of calling npm install
for the whole package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @kirgene, that's because we want all the charts to get published instead of only high-level charts.
So we need the dependencies to be populated in the respective charts folder for all the sub charts also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vijayg10 , am not sure I follow. All (sub)dependencies of high-level charts are deployed, otherwise, what's the point of the "dependency" feature in charts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helm does not update all dependencies for its dependency tree deeply, it will only do so at one level. Thus if you do not run the dep update for each chart (in the correct order), you will have missing dependencies.
At least this is how it worked in Helm v2 and the version I tested in v3. So if something has changed in later v3 versions, please let me know :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am not aware of any dependency problems in helm.
Could you describe how you performed those tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdebarros @vijayg10 please see the following repo for an example of using helm dependencies:
https://github.com/pm4ml/mojaloop-payment-manager-helm/blob/master/mojaloop-payment-manager/Chart.yaml
also, we don't need scripts/update-charts-dep.sh
:
https://github.com/pm4ml/mojaloop-payment-manager-helm/blob/c119c4c52222e794234cb1ed197e04da50494d35/.circleci/config.yml#L30-L44
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see that most bof chart dependencies are still 1.0.0, despite being updated frequently.
This obviously prevents fetching up-to-date changes from charts (and dependencies).
The correct way of handling these updates is:
- Modify chart, for example, bump docker image version in values file
- Bump Chart version
- Bump dependency version in the dependent Chart
- Bump dependent Chart version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This process is correct:
Modify chart, for example, bump docker image version in values file
Bump Chart version
Bump dependency version in the dependent Chart
Bump dependent Chart version
However, if we are publishing, we need to build the dependency tree manually, as this is not currently handled by Helm itself, references:
Helm is failing to download dependencies from external chart repos with this option specified, so we need to remove it.
One of such dependencies is reporting templates helm chart, which is included as a dependency for reporting service.
Removing this option shouldn't affect anything, as it is probably used for optimization purposes only.