Deprecate cr index "charts-repo" flag. Instead read index.yaml from git repository #144
Conversation
So far the index.yaml was loaded directly from the chart repository. In case of GitHub pages that's https://<user or org>.github.io/<repo> The problem with this approach is that GitHub pages use a Content Delivery Network. So the index.yaml files fetched from there might be outdated as there is a delay between a git push to the gh-pages branch until the content is visible. This delay might lead to the situation that one downloads an old index file from and updated that one instead of the most recent index.yaml. For the grafana repository that resulted in already released charts being removed again. - grafana/helm-charts#783 - grafana/helm-charts#796 This PR resolves that issue by reading the index.yaml from the gh_pages branch of the repository. The branch needs to be up-to-date anyhow if you want to create a PR or directly push the changes. It should also be fine for the chart-releaser-action as it uses a fetch-depth: 0 to clone the whole repository. ``` jobs: release: runs-on: ubuntu-latest steps: - name: Checkout uses: actions/checkout@v2 with: fetch-depth: 0 ``` Fixes: helm#143 Signed-off-by: Torsten Walter <torsten.walter@syncier.com>
Signed-off-by: Torsten Walter <torsten.walter@syncier.com>
There was a problem hiding this comment.
@torstenwalter good idea, and thank you! Nice implementation 👏
In order for us to include this in a PATCH release, we should not remove the charts-repo flag in this PR. Instead, we can deprecate it:
- mark as deprecated in the code
- hide the command flag from help output
- when the flag is used, add a CLI message to note this is now deprecated: quick explanation why it's is no longer necessary (index now drawn directly from the configurable gh pages branch to bypass gh periodic CDN delay etc), and let folks know it will be removed in the next MAJOR release
- regenerate/update usage text in README with the now hidden command
Removing the flag now will break existing CI that uses helm/chart-releaser directly. We can coordinate updating the helm/chart-releaser-action version to handle this properly when we bump the chart-releaser version used there, but that won't help users who rely on this project directly rather than using the gh action to wrap it.
|
@torstenwalter I also updated this PR title so the flag deprecation will show up clearly in the changelog |
|
@scottrigby and I was hoping to get rid of the compatibility as it makes the code way more complicated ;-) |
|
@torstenwalter I think you're right we don't need to fall back to old logic. If we mark the flag as deprecated and clarify it no longer does anything (now handled using a 100% backwards compatible technique of grabbing index from the branch rather than over CDN), we are still backwards compatible and can PATCH release. In 2.x we can make MAJOR changes like removing flags and other not 100% backwards-compatible changes. The alternative is to bump to 2.x now. I just think this one change can be made in a backwards-compatible way pretty easily. Do you agree? |
|
A context note: hip-0004 focuses on helm/helm, but the spirit is for the Helm project as a whole. Backwards compatibility may have been less rigorous from time to time for helm sub-projects, but to the best of our ability, we should keep the trust in helm subprojects (like helm/chart-releaser) by being as strict and transparent as we can about following semver's backwards compatibility rules for these sub-projects too 🙂 |
- message is printed when using the flag to let users know that it's deprecated - update all docs in README Signed-off-by: Torsten Walter <torsten.walter@syncier.com>
|
Updated as requested. I also upated other outdated documentation in the README. |
cr/cmd/index.go
Outdated
| if len(config.ChartsRepo) > 0 { | ||
| fmt.Fprintf(os.Stderr, "ATTENTION: Flag --charts-repo is deprecated. It does not have any effect.\n"+ | ||
| "The index.yaml is read from the '%s' branch instead.\n"+ | ||
| "Loading index.yaml directly from the charts repository lead to problems as there is a delay between\n"+ | ||
| "pushing to the github pages branch until things apear online.\n"+ | ||
| "The flag will be removed with the next major release.", config.PagesBranch) | ||
| } |
There was a problem hiding this comment.
I decided to print the deprecation message here. Alternative would have been the releaser.UpdateIndexFile() method.
Co-authored-by: Reinhard Nägele <unguiculus@gmail.com> Signed-off-by: Torsten Walter <mail@torstenwalter.de>
1bf9f8d to
14d1579
Compare
Signed-off-by: Torsten Walter <torsten.walter@syncier.com>
|
@scottrigby @unguiculus is it possible to create a new release with this change? |
|
i will review and see what more things we have and prepare a release |
So far the index.yaml was loaded directly from the chart repository. In case of GitHub pages that's https://.github.io/
The problem with this approach is that GitHub pages use a Content Delivery Network. So the index.yaml files fetched from there might be outdated as there is a delay between a git push to the gh-pages branch until the content is visible.
This delay might lead to the situation that one downloads an old index file from and updated that one instead of the most recent index.yaml.
For the grafana repository that resulted in already released charts being removed again.
This PR resolves that issue by reading the index.yaml from the gh_pages branch of the repository. The branch needs to be up-to-date anyhow if you want to create a PR or directly push the changes.
It should also be fine for the chart-releaser-action as it uses a fetch-depth: 0 to clone the whole repository.
Fixes: #143