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

Feature: add extra flags to skip helm dependencies download #541

Open
fredleger opened this issue Mar 31, 2023 · 32 comments
Open

Feature: add extra flags to skip helm dependencies download #541

fredleger opened this issue Mar 31, 2023 · 32 comments

Comments

@fredleger
Copy link

fredleger commented Mar 31, 2023

THIS IS A FEATURE REQUEST:

Actually running ct lint on limited connectivity work envs (air gap / train / plain / slow connectivity) give a limited experience, because ct lint is doing a helm depency build even if the awaited tgz are already present.
This also can in some cases lead to unwanted changes to be added to git HEAD (tgz modifications with same versions but different rights or just corrupted during download)

image

I would suggest adding a flag (and such option in ct.yaml) to allow skipping it.

I guess there are 2 possible ways to implement :

  • skip the helm dependency build if tgz are present, but we can't guarantee they are fresh enough
  • skip the whole step, relying on the user to do dependency build before running ct

what do you think ?

@Ornias1993
Copy link

What I think is that chart-testing developers are generally AWOL 99% of the time.

Copy link

github-actions bot commented Nov 3, 2023

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Nov 3, 2023
@fredleger
Copy link
Author

not stale (on my side)

@github-actions github-actions bot removed the Stale label Nov 5, 2023
Copy link

github-actions bot commented Dec 5, 2023

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Dec 5, 2023
@Ornias1993
Copy link

What is stale are the maintainers here.

@cpanato
Copy link
Member

cpanato commented Dec 5, 2023

@Ornias1993 Please be more kind and control your tone please. If you want to help, please help triage issues and review PRs and add fixes and features.

@Ornias1993
Copy link

Ornias1993 commented Dec 5, 2023

@Ornias1993 Please be more kind and control your tone please. If you want to help, please help triage issues and review PRs and add fixes and features.

If only triage, review etc got any attention from Helm maintainers.
Which, mater of factly isn't the case.

I also already filed a formal complaint towards the CNCF about the lack of activity of the maintainers of Helm, with the ever increasing load of issues and PR's that don't get any attention at-all.

I think the fact you responded on my tone, with the backlog of issues/PRs needing your attention growing, made my case for me.

@github-actions github-actions bot removed the Stale label Dec 6, 2023
@scottrigby
Copy link
Member

@Ornias1993

Expressing differing opinions, viewpoints, and experiences is welcome. Making trolling, insulting or derogatory comments is not.

This Helm subproject follows the CNCF Code. Please read this in full. Here are a few of the examples of behavior that contributes to a positive environment:

  • Giving and gracefully accepting constructive feedback
  • Accepting responsibility and apologizing to those affected by our mistakes, and learning from the experience

To address your actual issue - as @cpanato said - if you feel the backlog of issues and PRs for this free software is too long, consider helping to reproduce, test, and review them. I acknowledge we need more help with this from community members than we currently have. If you decide to try helping, let me know and I will personally help guide you through the process.

@scottrigby
Copy link
Member

Leaving this issue open to focus on @fredleger's feature request.

@cpanato
Copy link
Member

cpanato commented Dec 14, 2023

@fredleger apologies for the delay, I will try to prioritize this work and added that to my backlog for my next cycle

@Ornias1993
Copy link

@Ornias1993

You for taking responsibility for your mistakes in the mater.

Sadly I wont be able to contribute as al my development time goes into my own project.

Including needing to write custom code for processing helm charts en-masse (900 of them), as Helm tooling has significant performance issues.

Basically I dont have the time left to contribute back to helm, because I already need to spend devtime compensating for helm issues. So quite a catch 22.

@scottrigby
Copy link
Member

@fredleger Have you tried to test and benchmark this process with charts and dependent charts all stored in OCI? This does not address the feature request for Helm HTTP repos, but it may solve your issue since OCI registries do not work with a single index file like HTTP Helm repos do.

@Ornias1993 this may help your scaling issues too:

Including needing to write custom code for processing helm charts en-masse (900 of them), as Helm tooling has significant performance issues.

If either of you want to discuss this further, we can do so here or though the other Helm communication channels. Specifically, I would encourage you both to reach out on Slack (for async or real time chat), and to join the live weekly Helm meetings. The next Helm dev meeting is in 5 minutes. All the info on how to join these is here: https://github.com/helm/community/blob/main/communication.md

@Ornias1993
Copy link

@scottrigby We've since almost a year moved to a custom cached dependency fetcher (which I just migrated to go for an even more significant performance improvement).

The issue wasn't the yaml index per-se (though that does take a part)...
The repeated uncached downloads where a far worse issues. Spending a few seconds times 900 charts, is a huge problem.

But we solved it and are this year starting to develop custom tooling for more of our pipelines.
Thats not a fault by Helm btw, it's expected that once you reach a certain scale that "off the shelve" tools start hitting limits.

@fredleger
Copy link
Author

@fredleger Have you tried to test and benchmark this process with charts and dependent charts all stored in OCI? This does not address the feature request for Helm HTTP repos, but it may solve your issue since OCI registries do not work with a single index file like HTTP Helm repos do.

@Ornias1993 this may help your scaling issues too:

Including needing to write custom code for processing helm charts en-masse (900 of them), as Helm tooling has significant performance issues.

If either of you want to discuss this further, we can do so here or though the other Helm communication channels. Specifically, I would encourage you both to reach out on Slack (for async or real time chat), and to join the live weekly Helm meetings. The next Helm dev meeting is in 5 minutes. All the info on how to join these is here: https://github.com/helm/community/blob/main/communication.md

@scottrigby if you look at my original post the issue arrise when using bitnami deps which is from my experience one of the most common use case. Then unless bitnami migrate all their charts to oci registry or if I deploy a mirror registry with oci enabled I don't see how this could help me.

Don't hesitate to correct me if I am wrong.

@fredleger
Copy link
Author

And for me the point is not the slowness of the download but the fact the download occur at every CT lint

@Ornias1993
Copy link

And for me the point is not the slowness of the download but the fact the download occur at every CT lint

This is why we stopped using ct lint completely.
first it redownloads the index, then it also redownloads the dependency.

If your scope increases byond a handfull of charts these slowdowns become unworkable.
OCI is nice and all, but it just solves the first portion (redownloading index)

@scottrigby
Copy link
Member

Well, OCI solves more than that. But for this particular problem with scaling dependency testing, I see what you mean. Thanks for clarifying.

Question about this:

I would suggest adding a flag (and such option in ct.yaml) to allow skipping it.

If we implemented this way, would relying on the user to do the dep build solve your use case, or would that not be quite enough?

skip the whole step, relying on the user to do dependency build before running ct

Something like a new --skip-dependency-build option to ct lint. If set, would effectively skip this section of the code that does the dependency downloading:
https://github.com/helm/chart-testing/blob/main/pkg/chart/chart.go#L365-L401

And what would propagating this option to the wrapping GitHub action look like? https://github.com/helm/chart-testing-action

@scottrigby
Copy link
Member

Possibly relevant to this discussion:

@fredleger
Copy link
Author

If we implemented this way, would relying on the user to do the dep build solve your use case, or would that not be quite enough?

for me it would fix the original issue yes. My point was not related to using ct lint at scale which is IMHO another issue

@scottrigby
Copy link
Member

@scottrigby if you look at my original post the issue arrise when using bitnami deps which is from my experience one of the most common use case. Then unless bitnami migrate all their charts to oci registry or if I deploy a mirror registry with oci enabled I don't see how this could help me.
Don't hesitate to correct me if I am wrong.

@fredleger Replying to this note, you'll be pleased to know all Bitnami charts have been Generally Available as OCI artifacts since spring of this year ✨ https://blog.bitnami.com/2023/04/httpsblog.bitnami.com202304bitnami-helm-charts-now-oci.html

@scottrigby
Copy link
Member

Also relevant to the discussion, this fix is scheduled as part of the Helm 3.14.0 release milestone in January:

@scottrigby
Copy link
Member

@fredleger, thanks for this clarification.

for me it would fix the original issue yes. My point was not related to using ct lint at scale which is IMHO another issue

@Ornias1993, would this fix your issue as well? Or would the helm dep up performance PR above fix your problem?

If not, please create a more specific issue for yours, and link to this one so we can track which use cases are covered and which aren't.

@scottrigby
Copy link
Member

@fredleger would you like to try your hand at a PR for this? If so, I'd be happy to guide you.

Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jan 27, 2024
@fredleger
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label Feb 1, 2024
Copy link

github-actions bot commented Mar 3, 2024

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Mar 3, 2024
@fredleger
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label Mar 5, 2024
Copy link

github-actions bot commented Apr 4, 2024

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Apr 4, 2024
@fredleger
Copy link
Author

Not stale

@github-actions github-actions bot removed the Stale label Apr 5, 2024
@SuperSandro2000
Copy link

I've just wanted to join the group of interested people in this and fight away the stale bot.

@cpanato
Copy link
Member

cpanato commented Apr 11, 2024

@fredleger @SuperSandro2000 are you willing to work on this issue and open a PR?

@fredleger
Copy link
Author

@cpanato i don't have much skills in GO so passing my turn here

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

No branches or pull requests

6 participants