[BUG] Helm repo add not working as expected

[peterzhuamazon]

Describe the bug
Helm repo add not working as expected in https://github.com/opensearch-project/helm-charts#installation

To Reproduce
$ helm repo add opensearch https://opensearch-project.github.io/helm-charts/
Error: looks like "https://opensearch-project.github.io/helm-charts/" is not a valid chart repository or cannot be reached: failed to fetch https://opensearch-project.github.io/helm-charts/index.yaml : 404 Not Found

Expected behavior
Helm repo added

Chart Name
All

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• Helm Version: [version.BuildInfo{Version:"v3.7.0", GitCommit:"eeac83883cb4014fe60267ec6373570374ce770b", GitTreeState:"clean", GoVersion:"go1.16.8"}]
• Kubernetes Version: [Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.1", GitCommit:"5e58841cce77d4bc13713ad2b91fa0d961e69192", GitTreeState:"clean", BuildDate:"2021-05-21T23:01:33Z", GoVersion:"go1.16.4", Compiler:"gc", Platform:"linux/amd64"}]

Additional context
Add any other context about the problem here.

[peterzhuamazon]

@mprimeaux could you take a look on this?
Thanks.

[mprimeaux]

@peterzhuamazon It appears the GitHub pages branch is private or not yet setup. Would you mind confirming that the "Pages" settings on this repository is set correctly. For example, this is how I have my fork repository set for https://mprimeaux.github.io/helm-charts/

[mprimeaux]

You will first need to create a branch named gh-pages and push it. Then the branch will be available in the "Pages" settings of this repository.

[mprimeaux]

@peterzhuamazon It seems the chart releaser workflow is failing.

Essentially, what happened is the initial merge to main failed because the gh-pages branch was not created before merging PR #46, which caused this failure.

Then PR #19 was merged without incrementing the chart version, which caused the subsequent actions workflow to fail.

Recall that we enabled the validation of the Helm chart version in compliance with Semver 2; it MUST be incremented with each change. PR #19 should have incremented the chart version.

To fix this, we will need to:

1. Create the gh-pages branch as per above steps.
2. Increment the Chart version of the OpenSearch-Dashboard chart to 1.0.2.
3. Merge to main.

I can submit a PR to compensate for PR #19 but I of course do not have permissions to activate GitHub Pages on this repository. Please let me know. Happy to help.

Here is an example of it working as documented against my forked repository when the actions workflow succeeds:

❯ helm repo add opensearch https://mprimeaux.github.io/helm-charts/
"opensearch" has been added to your repositories

❯ helm search repo opensearch
NAME                              	CHART VERSION	APP VERSION	DESCRIPTION
opensearch/opensearch             	1.0.1        	1.0.0      	A Helm chart for Kubernetes
opensearch/opensearch-dashboards  	1.0.1        	1.0.0      	A Helm chart for OpenSearch Dashboards

[peterzhuamazon]

Thanks @mprimeaux for the detailed comments.
Thanks for that.

I will try to enable this tomorrow.

[mprimeaux]

Here is a minor update to watch. While I do not believe it applies to us since we have a public repository, we ran into the issue in our private repository.

[peterzhuamazon]

Hi @mprimeaux another question if you dont mind.
Please bare with me as I am still learning about this cr thing.

I assume gh-pages branch is synced with main.
Then is there any reasons to create branches like 1.0.0 for OpenSearch 1.0.0, and so on?
If user wants to install specific version such as 1.0.0, 1.0.1, or later 1.1.0, will this be supported by helm repo?

If so, how do we approach to enable that?

---

PS:

I also realize our description of opensearch helm chart is still:

description: A Helm chart for Kubernetes

Probably needs to rename it once I bump versions.

Thanks.

[mprimeaux]

@peterzhuamazon No worries. Happy to provide more detail.

The Chart Releaser Github action is what pushes the index to the gh-pages branch and after creating releases for the related charts. The workflow is:

• Linting and testing on all branches and PRs that are not main and not gh-branches
• Releasing on all pushes to main.

There is not a need to create and maintain version-specific branches but more of a stylistic question to be answered by the maintainers. The Chart Releaser tags with the version number and then generates release artifacts. Here is an example.

Yes, agree the description should be updated.

[peterzhuamazon]

Thanks @mprimeaux this does raise a concern from me.

The release show as opensearch-dashboards-1.0.2 and so on which is chart version, internally use dashboards 1.0.0.
This is quite confusing to users as they can probably take it as dashboard version 1.0.2.
In reality we havent even release dashboards 1.0.2 yet.

I am wondering if we can have a fix to this before we enable the repo?
I can already see people opening issues asking why opensearch-dashboards-1.0.2 install 1.0.0.

[mprimeaux]

@peterzhuamazon The reason for the versions at 1.0.2 is because I was playing around in my forked repository. You can see I updated the chart versions here and here (again, in my forked repository).

One option to avoid confusion is for me to delete my forked repository or at least make it private.

[TheAlgo]

@mprimeaux @peterzhuamazon Till we have it working end to end can we revert the commit for linting and releasing as we have put the wrong installation instructions in the README. Once we fix we can re-merge.

[peterzhuamazon]

@peterzhuamazon The reason for the versions at 1.0.2 is because I was playing around in my forked repository. You can see I updated the chart versions here and here (again, in my forked repository).

One option to avoid confusion is for me to delete my forked repository or at least make it private.

I understand that is your own fork, but still if Semver version bumping is required now, you will have situation where opensearch-1.0.9 is actually using docker image 1.0.0 internally. And if the release show as opensearch-1.0.9 community can mistakenly think it is running docker image 1.0.9 which doesnt exist.

[mprimeaux]

@peterzhuamazon I would sincerely prefer to work through this simple fix, which I described in this comment.

The version attribute of the chart is on a separate lifecycle from the appVersion and the imageTag in the values.yaml.

A way forward would be to disable the version checking in the Helm Chart releaser action but, as I agree with @smlx , it is critical to enforce Semver in charts so the community using it understands the impact of changes.

Can we please work through this?

[mprimeaux]

I understand that is your own fork, but still if Semver version bumping is required now, you will have situation where opensearch-1.0.9 is actually using docker image 1.0.0 internally. And if the release show as opensearch-1.0.9 community can mistakenly think it is running docker image 1.0.9 which doesnt exist.

@peterzhuamazon This is usual as the version is the Chart's version, appVersion is the backing "application's" version and the image attribute in the values.yaml is the container image's version (i.e. Docker image tag).

The Chart can be at version: 1.0.1, appVersion: 2.0.0, and imageTag: 1.0.0 and this expected and usual. It is important that these lifecycles be capable of being managed separately.

My opinion is we do not want the community to think they are using the same version of the chart when changes to the chart have been made.

[TheAlgo]

@mprimeaux

The Chart can be at version: 1.0.1, appVersion: 2.0.0, and imageTag: 1.0.0 and this expected and usual. It is important that these lifecycles be capable of being managed separately.

I think chart version and imageTag should match. It makes things clearer and simpler, and I don't care much about the appVersion though.

[peterzhuamazon]

@TheAlgo and I have some concerns on release happen on every small changes.
It is a moving target on every release and it becomes uncertain if the release is stable.
We can probably remove the auto release workflow and just release manually, but keep the repo function of helm.

@DandyDeveloper @smlx please let us know your thoughts on this.
I apologize that I am not fully aware of the complete picture initially.
Should we keep bumping versions and release every push, or we should proceed with a more stable way.

[mprimeaux]

I reel chart version and imageTag should match. It makes things clearer and simpler, and I don't care much about the appVersion though.

Please take this comment as nothing but constructive as it is often difficult to understand tone and intention in written words but this is not how the Kubernetes community broadly works. I can provide countless examples (i.e. here and here for the kube-prometheus-stack and here and here for the AWS Load Balancer Controller) where there is an expectation that the lifecycles of the Helm chart, the application, and the related image remain distinct because they do operate at different frequencies.

I sincerely appreciate your understanding and hearing me out on this point. Please let me know how I can support as many of us are eager to use the Helm charts.

[TheAlgo]

I reel chart version and imageTag should match. It makes things clearer and simpler, and I don't care much about the appVersion though.

Please take this comment as nothing but constructive as it is often difficult to understand tone and intention in written words but this is not how the Kubernetes community broadly works. I can provide countless examples (i.e. here and here for the kube-prometheus-stack and here and here for the AWS Load Balancer Controller) where there is an expectation that the lifecycles of the Helm chart, the application, and the related image remain distinct because they do operate at different frequencies.

I sincerely appreciate your understanding and hearing me out on this point. Please let me know how I can support as many of us are eager to use the Helm charts.

Not sure if I am understanding correct but the chart version and the underlying docker image version is matching for 2 latest versions of AWS Load Balancer Controller.

[mprimeaux]

@TheAlgo I believe you are comparing the appVersion and imageTag attributes rather than the version and imageTag attributes, which are 1.2.8 and v2.2.4, respectively.

[TheAlgo]

@mprimeaux Apologies I believe I mis-interpreted at first 😅. Need to restart myself. appVersion and imageTag I wanted to refer to at start.

I think chart version and imageTag should match. It makes things clearer and simpler, and I don't care much about the appVersion though

Does this sound good?

[mprimeaux]

@TheAlgo No worries, mate. Ah yes, that sounds good. I do believe quite a few Helm charts do also prefer to keep the appVersion and imageTag the same for simplicity. IMHO, and to reduce the conversation a bit, I think it is important to allow the chart's version to evolve at a different pace than that of appVersion and imageTag.

[mprimeaux]

@TheAlgo As a follow-up and for completeness, the Helm Chart Releaser only validates the version attribute in Chart.yaml and does not consider the appVersion or imageTag.

[DandyDeveloper]

Assuming I'm reading right, I agree wholly with @mprimeaux on this. Version is distinctly different from the imageTag referenced by the appVersion and its much easier to trace changes and automated things if we define chart version increments in a PR.

Example, just to be clear. I make a PR updating the StatefulSet with some simple changes, and I update the Chart.yaml -> version field from 1.0.0 to 1.0.1 as this is a simple patch. Once merged, GitHub actions auto deploys and releases this new version (we could arguably also create a GitHub release for that, which references the issue/PR that made the change).

That way, when the PR is merged, we have a history of where something broke and exactly what happened to break it.

The linting from ct lint inherently supports this by default as well. As does the chart releaser.

If I'm misunderstood, disregard and let me know. I'll read again and give a better more coherent answer.

[smlx]

I agree with @DandyDeveloper and @mprimeaux. The Helm community and tooling is highly opinionated regarding development and release workflow:

• Chart version is critically important and must follow SemVer. Chart versions are immutable and version bumps are required for any change. Major version bumps are required for breaking changes. Consumers of the chart rely on this to determine upgrade safety. Review of PRs should also consider if the version is bumped correctly.
• appVersion is totally decoupled from chart version and simply indicates the default image tag.
• imageTag is only there for end users to override the default appVersion of a chart, if necessary. It should not be populated in the default values.yaml.

The prometheus community repo is a good example of how the ct and cr tooling is supposed to work: https://github.com/prometheus-community/helm-charts

See in particular their contribution guidelines: https://github.com/prometheus-community/helm-charts/blob/main/CONTRIBUTING.md

[TheAlgo]

@smlx @mprimeaux @DandyDeveloper I understand the view point of some of the community charts.

Also,
Chart version can be bumped but we should have a description of the Changelog we did. We should have a consensus on what changes we should bump. Also it should mention the underlying docker image powering it. Sometimes people think those versions as OpenSearch/Dashboards versions so a proper description will suffice the confusions.

@anuragsarkar97 opinions on this?

[mprimeaux]

@TheAlgo I agree and recommend for consideration that we embrace the Keep A Change Log format, which, in a nutshell, covers:

• Added for new features.
• Changed for changes in existing functionality.
• Deprecated for soon-to-be removed features.
• Removed for now removed features.
• Fixed for any bug fixes.
• Security in case of vulnerabilities.

This format also subscribes to Semver.

[mprimeaux]

@peterzhuamazon @TheAlgo @smlx @DandyDeveloper If you are supportive, I will submit a PR tomorrow (US Central Time) to address this thread and issue. I appreciate your consideration. If you prefer a different tact, then please let me know.

[peterzhuamazon]

In the meantime @TheAlgo has raised a #60 PR so that users wont get confused on issues with helm add.
They can still use the old package method to install.

[TheAlgo]

@TheAlgo I agree and recommend for consideration that we embrace the Keep A Change Log format, which, in a nutshell, covers:

Added for new features.
Changed for changes in existing functionality.
Deprecated for soon-to-be removed features.
Removed for now removed features.
Fixed for any bug fixes.
Security in case of vulnerabilities.
This format also subscribes to Semver.

This is neat and clean @mprimeaux . I am fine with this.

[mprimeaux]

Please ignore PR #62 as it was an inadvertent mistake on my part. PR #61 is ready for review. Thanks again for your help and support.

[peterzhuamazon]

Hi @mprimeaux this page is now working but still could not add helm repo
https://opensearch-project.github.io/helm-charts/

Error: looks like "https://opensearch-project.github.io/helm-charts/" is not a valid chart repository or cannot be reached: failed to fetch https://opensearch-project.github.io/helm-charts/index.yaml : 404 Not Found

Seems like the yaml still not existing.

Thanks.

[mprimeaux]

Thanks, @peterzhuamazon . I will review it now. Would you please confirm the gh-pages branch is not set to private access?

[mprimeaux]

The chart releaser thought there was nothing to do as per this step and so the fix is to increment that patch version for each chart.

I'll submit a PR after testing in my fork, which I'll do now.

[peterzhuamazon]

It works @mprimeaux:

$ helm repo add opensearch https://opensearch-project.github.io/helm-charts/
"opensearch" has been added to your repositories

$ helm repo update
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "opensearch" chart repository
...Successfully got an update from the "opensearch-mprimeaux" chart repository
Update Complete. ⎈ Happy Helming!⎈

$ helm repo list
NAME                	URL
opensearch          	https://opensearch-project.github.io/helm-charts/

$ helm search repo opensearch
NAME                                      	CHART VERSION	APP VERSION	DESCRIPTION
opensearch/opensearch                     	1.0.3        	1.0.0      	A Helm chart for OpenSearch
opensearch/opensearch-dashboards          	1.0.3        	1.0.0      	A Helm chart for OpenSearch Dashboards

[mprimeaux]

Great news! I sincerely appreciate you and all of the maintainers diligence and help. Once you're comfortable then I will submit a PR with the new installation instructions.

[peterzhuamazon]

Thanks everyone in the thread for help, especially @mprimeaux who put such efforts into getting it running.
I will close this issue soon as we can already install it.
However, I want to check out if there is any extra steps beside the installation instructions that we want to add?

Thanks.

[mprimeaux]

However, I want to check out if there is any extra steps beside the installation instructions that we want to add?

I am not aware of any other steps in this repository other than to update the installation steps. I do see the OpenSearch docs also would need to be updated to reflect the new installation steps but believe this is outside the scope of this repo.

Will wait for @smlx @DandyDeveloper and @TheAlgo to review.

[peterzhuamazon]

I will contact the doc team to make the changes on OpenSearch docs once we have installations steps ready in this repo, so they can update with the same process.

Thanks.

[mprimeaux]

Thanks, @peterzhuamazon. In advance, I'll submit a PR for review. Should any of the maintainers prefer additional changes then I'm happy to address in the PR.

Again, thanks for your support and coordination.

[mprimeaux]

@peterzhuamazon I did notice that CHANGELOG.md needs to reflect chart versions 1.0.3 so I'll add that to the PR for alignment.

Question is do we want to remove CHANGELOG.md from the root since we have CHANGELOGs for each chart in their respective folders? Less maintenance to remove the root one since the README already points to the respective folders.

[mprimeaux]

@peterzhuamazon There are a few other observations:

1. The opensearch-dashboard chart does not have a README.md in its directory.
2. There is a README.md in the /charts directory but it looks fairly out of date or, at least, needs a bit of cleanup.

I'll address bullet 1 in the PR. However, it seems to me that we'd want to merge the contents of the README referring to in bullet 2 into the repository root README.md. Thoughts on this?

[peterzhuamazon]

@peterzhuamazon I did notice that CHANGELOG.md needs to reflect chart versions 1.0.3 so I'll add that to the PR for alignment.

Question is do we want to remove CHANGELOG.md from the root since we have CHANGELOGs for each chart in their respective folders? Less maintenance to remove the root one since the README already points to the respective folders.

If each chart folder has a corresponding changelog, I am in support of removing the root changelog.

[peterzhuamazon]

@peterzhuamazon There are a few other observations:

1. The `opensearch-dashboard` chart does not have a `README.md` in its directory.

2. There is a `README.md` in the [/charts](https://github.com/opensearch-project/helm-charts/blob/main/charts/README.md) directory but it looks fairly out of date or, at least, needs a bit of cleanup.

I'll address bullet 1 in the PR. However, it seems to me that we'd want to merge the contents of the README referring to in bullet 2 into the repository root README.md. Thoughts on this?

I think @TheAlgo worked on the README of charts, needs his input on 2 here.

[mprimeaux]

I think @TheAlgo worked on the README of charts, needs his input on 2 here.

Absolutely. The content of the README in the charts folder seemed more applicable to the opensearch chart rather than tp the opensearch-dashboards charts so I 'took a chance' but expected a review. No worries at all.

[TheAlgo]

I think @TheAlgo worked on the README of charts, needs his input on 2 here.

Absolutely. The content of the README in the charts folder seemed more applicable to the opensearch chart rather than tp the opensearch-dashboards charts so I 'took a chance' but expected a review. No worries at all.

Sorry @peterzhuamazon @mprimeaux I missed this thread completely. I guess we should add links to different charts in that README.md and have separate README for opensearch and opensearch-dashboards. I worked on the opensearch README and would spare some time to create one for opensearch-dashboards as well. In case anyone is willing to help earlier I will be happy to help it go through.

[sastorsl]

Is this issue ready now for closure?
I.e. "Helm repo add not working as expected."

helm repo add seems to work now.

[mprimeaux]

@TheAlgo No worries.

We currently have separate CHANGELOGs and READMEs for both charts. i.e. OpenSearch README.md and CHANGELOG.md and OpenSearch Dashboards README.md and CHANGELOG.md.

We also reference these in the root README.md.

I think we should remove the root-level CHANGELOG.md, which I elevated for discussion in #107.

This issue can be closed.

[TheAlgo]

@mprimeaux I agree on removing the root level README.

Closing this issue for now as the issue is already resolved.