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

Add option to host the chart package files in the GitHub Pages branch #123

Merged
merged 8 commits into from
Jun 20, 2023

Conversation

annabarnes1138
Copy link
Contributor

When using this tool on a private repo, helm is unable to download the chart package files. When you give Helm your username and password it uses it to authenticate to the repository (the index file). The index file then tells Helm where to get the tarball. If the tarball is hosted in some other location (Github Releases in this case) then it would require a second authentication (which Helm does not support). The solution is to host the files in the same place as your index file and make the links relative paths so there is no need for the second authentication. This PR adds a flag (--packages-with-index) to the upload and index commands to accomplish this. It would resolve #115

@cpanato
Copy link
Member

cpanato commented May 17, 2021

@stecky thanks for this PR, can you please sign the DCO and add some tests for this change? thanks!

@annabarnes1138
Copy link
Contributor Author

ok @cpanato I signed the DCO and added some tests

@annabarnes1138
Copy link
Contributor Author

I added a patch to the PR to use the token passed in to authenticate to the repo when checking for an existing index.yaml

@sivakov512
Copy link

Is it planned to be merged? It would be great to start using it.

@pratikbin
Copy link

Is it merging anytime soon?

@brian-pickens
Copy link

@davidkarlsen I am also waiting on this functionality. Looks like a review is required to complete the merge.

@tomaszdudek7
Copy link

@cpanato any chance of merging it?

@ArchiFleKs
Copy link

Just hit this issue also, would love to see this merged

@cpanato
Copy link
Member

cpanato commented Sep 8, 2021

will add this to my backlog to perform some manual tests and check.

meantime @davidkarlsen @unguiculus @scottrigby any thoughts?

@javiersvg
Copy link

Just to bare in mind I found an issue with this approach while testing on the branched repository.
The problem is with the pushToPagesBranch method in releaser.go.
When that method is called to publish the chart to the gh-page branch and there is more that one chart (like when you have multiple subcharts) the push will fail committing the second chart. The cause for this is that the commits are being pushed directly from the current branch (main) to gh-pages branch.
I worked around this issue by separating the subcharts in multiple repos but I guess it is not ideal.
I think the solution would be to add to that method a logic to move to the gh-pages branch before making the commits and that way avoiding the conflict but haven't been able to put time to it.
Hope this helps a little.

@kristof-mattei
Copy link

I find this overly complex. I am able to use this application (through the action) as follows:

      - name: Run chart-releaser
        uses: ./.github/actions/helm/chart-releaser-action
        env:
          CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"
          CR_RELEASE_NAME_TEMPLATE: "helm-chart-{{ .Version }}"
        with:
          charts_dir: charts
          # see https://github.com/helm/chart-releaser/issues/115
          # needed for private GitHub repos
          charts_repo_url: https://raw.githubusercontent.com/owner/repo/gh-pages/

@shyam-ks
Copy link

When using this tool on a private repo, helm is unable to download the chart package files. When you give Helm your username and password it uses it to authenticate to the repository (the index file). The index file then tells Helm where to get the tarball. If the tarball is hosted in some other location (Github Releases in this case) then it would require a second authentication (which Helm does not support). The solution is to host the files in the same place as your index file and make the links relative paths so there is no need for the second authentication. This PR adds a flag (--packages-with-index) to the upload and index commands to accomplish this. It would resolve #115

When its done, Please add an example on how to use it . Thanks :)

@jinnjwu
Copy link

jinnjwu commented Oct 23, 2021

When merge it ? we really need it.

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

can we add some documentation to explain how to use that?

thanks for this PR

@cartmast-aws
Copy link

Also hitting this as a blocker in our private repository. Anything I can help with to get this merged?

@morganchristiansson
Copy link

I am also waiting for this PR as I'm unable to use this tool currently.

@DeepDiver1975
Copy link

for the time being use stecky/yet-another-chart-releaser-action@v1 - works perfect for me - THX @stecky

      - name: Run chart-releaser
        uses: stecky/yet-another-chart-releaser-action@v1
        with:
          packages_with_index: true
        env:
          YACR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"

@morganchristiansson
Copy link

Great thank you! And https://github.com/stecky/yet-another-chart-releaser already has documentation

@cartmast-aws
Copy link

The solution from stecky works great (thank you stecky), but this is clearly an in-demand feature that should be supported by the official Helm implementation.

Taking a core dependency on a fork of an official release branch in someone's personal github is at best a risky solution for anyone, and at worst a disaster for anyone who relies on this and the personal account decides to delete their repo/fork.

So I will ask again: is there anything I can do to facilitate and help get this PR merged so we can all use the official Helm chart releaser?

@GoSiddhartha
Copy link

GoSiddhartha commented Nov 23, 2021

The solution from stecky gives me the same error I get with the original chart-releaser.
image

Can someone help me, I am really stuck with this. Please help!!!

This is the repo setup -
image

With stecky the .tgz files are getting created

@sconnole
Copy link

Thanks to all who have been working on this thus far. Would love to have this fix merged in ASAP. Hopefully I can schedule some time in my day to look into this as well.

@sconnole
Copy link

sconnole commented Apr 3, 2023

Got hung up on needing to set my go path on this new laptop. 🙄 Pasting here in case someone else needs that info as well.

export PATH=$PATH:$(go env GOPATH)/bin

Replicated the same issue that @jgiretimhaus was having. I'll look more today to see if I can find a resolution.

@sconnole
Copy link

sconnole commented Apr 3, 2023

@jgiretimhaus you need to add --push to cr upload

cr upload --owner <OWNER> --git-repo helm-test --packages-with-index --token <TOKEN> --skip-existing --push

image

my index.yaml

image

@jgiretimhaus
Copy link
Contributor

Hey @sconnole ! I was away for work yesterday and didn't see your ping.

But god damn was I happy this morning ahaha ! I tried adding the --push flag in the upload method and it worked as expected. I was sure it was something like that :) Thanks a lot for taking the time to test it and finding the flag I was missing !

Here is my github-pages branch :

ghbranch

And my updated index.yaml :

indexyaml

Really excited to finally get this feature added ! Bravo to everyone that worked on this ! @cpanato can we expect a merge and a release in the coming week ?

@tontongg
Copy link

Hi @cpanato ! Thanks again for your work. Do you have any release date for this feature please ?
Thanks a lot

@tontongg
Copy link

tontongg commented May 9, 2023

Hi, sorry for being insistent :-) trying to get any news from @cpanato
thanks in advance

@jgiretimhaus
Copy link
Contributor

jgiretimhaus commented May 23, 2023

Hello @cpanato, sorry to ping you again as I am sure that you already have plenty on your plate but this feature would be a game changer for us and it is so close to completion, requiring only a merge that it would be a shame if the issue became stale again. Many thanks again !
7msj1h

@edijsdrezovs
Copy link

have

Any news when we could expect this to be merged?

annabarnes1138 and others added 5 commits June 5, 2023 13:18
Signed-off-by: Steven Barnes <Steven.Barnes@topgolf.com>
Signed-off-by: Steven Barnes <Steven.Barnes@topgolf.com>
Signed-off-by: Steven Barnes <Steven.Barnes@topgolf.com>
Signed-off-by: Steven Barnes <Steven.Barnes@topgolf.com>
Signed-off-by: cpanato <ctadeu@gmail.com>
@cpanato cpanato force-pushed the packages-with-index branch from 5f864f8 to 1ff032d Compare June 5, 2023 11:19
cpanato added 2 commits June 5, 2023 13:24
Signed-off-by: cpanato <ctadeu@gmail.com>
Signed-off-by: cpanato <ctadeu@gmail.com>
@cpanato cpanato force-pushed the packages-with-index branch from 1ff032d to 90859b0 Compare June 5, 2023 12:19
Signed-off-by: cpanato <ctadeu@gmail.com>
@cpanato
Copy link
Member

cpanato commented Jun 5, 2023

@jgiretimhaus @edijsdrezovs funny image :) but I am a bit on the fence about merging this, but let's do that, I did some manual tests, and it seems to work and not break the existing behavior.

@jgiretimhaus
Copy link
Contributor

jgiretimhaus commented Jun 5, 2023

@jgiretimhaus @edijsdrezovs funny image :) but I am a bit on the fence about merging this, but let's do that, I did some manual tests, and it seems to work and not break the existing behavior.

Hey, @cpanato-Kenobi ^^ ! Thanks a lot for the big push ! Do you have a specific part of the PR that worries you or do you expect any unwanted behavior that we should look out for and report ? As we keep testing and start bringing this to our production slowly ofc so we can monitor this, I'll be sure to let you guys know if we encounter any problem, here, in this thread.

We should maybe still keep the issue open a bit as we let's say beta test this in production for a month maybe ?

Will the chart-releaser-action be updated when the merge passes ?

Awesome work and thanks again to you and @stecky for bringing this feature to the community !

Cheers !

Copy link
Member

@davidkarlsen davidkarlsen left a comment

Choose a reason for hiding this comment

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

lgtm.
chart-releaser can run in several modes now, so maybe we should add some more docs in the readme to verbose those usages out - but that can be added later on, as this change is backwards compatible.

@cpanato
Copy link
Member

cpanato commented Jun 15, 2023

@jgiretimhaus @edijsdrezovs looks like you both are the most interested in this feature. Can I have a commitment from all of you to create/update/add some documentation to cover both flows? as @davidkarlsen points out?

if you agree, we can merge this with a promise that we will have some documentation. wdyt?

@valeriano-manassero
Copy link

@jgiretimhaus @edijsdrezovs looks like you both are the most interested in this feature. Can I have a commitment from all of you to create/update/add some documentation to cover both flows? as @davidkarlsen points out?

if you agree, we can merge this with a promise that we will have some documentation. wdyt?

I'm also interested so maybe I can help with a PR in some time.

@jgiretimhaus
Copy link
Contributor

@jgiretimhaus @edijsdrezovs looks like you both are the most interested in this feature. Can I have a commitment from all of you to create/update/add some documentation to cover both flows? as @davidkarlsen points out?

Of course :) ! I should have some time today, I'll try to start a nice little verbose documentation today

I'll keep you updated

@jgiretimhaus
Copy link
Contributor

Hey ! I was able to work on it today.

I added a section about the usage of the --package-with-index flag and an example reusing the screenshots I took while testing this PR.

I also added a Common Error Messages section as I encountered a 422 Validation Failed while retesting the whole process to make sure the process described in the documentation was working as intended. It is solved using the --skip-existing flag.

Btw, you could close issue #101 as it is solved in #111 that adds the --skip-existing flag.

Tell me if I should update/remove/add something so it fits better.

My PR is here and DCO is signed :)

Looking forward to the merge. Cheers !

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cr index does not work with github enterprise without anonymous access