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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove --skip-refresh option from scripts/update-charts-dep.sh #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/update-charts-dep.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

@mdebarros mdebarros Dec 9, 2021 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@kirgene kirgene Dec 9, 2021 •
edited
Loading

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.

Copy link
Member

@mdebarros mdebarros Jan 12, 2022 •
edited
Loading

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@kirgene kirgene Jan 25, 2022 •
edited
Loading

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

Copy link
Contributor Author

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:

  1. Modify chart, for example, bump docker image version in values file
  2. Bump Chart version
  3. Bump dependency version in the dependent Chart
  4. Bump dependent Chart version

Copy link
Member

@mdebarros mdebarros Jan 25, 2022 •
edited
Loading

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:

done

set +x
Expand Down