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

Jump to bottom

fix(helm): Accept dependency in requirements.yaml from charts directory #3422

Closed
wants to merge 9 commits into from
Closed

fix(helm): Accept dependency in requirements.yaml from charts directory #3422

wants to merge 9 commits into from

Conversation

splisson
Copy link
Contributor

@splisson splisson commented Jan 30, 2018 •
edited by bacongobbler
Loading

This change allow the use of charts from the charts sub-directory of the current chart in requirements.yaml. It checks the version. It leaves the chart in place.

Closes #3221

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 30, 2018
@jascott1
Copy link
Contributor

HI @splisson

Thanks for the PR! Can you update this to specify which bug or feature proposal this is related to? See the format here https://github.com/kubernetes/helm/blob/master/docs/developers.md#git-conventions

@splisson splisson changed the title #3321 Accept dependency in requirements.yaml from charts directory. fix(helm): Accept dependency in requirements.yaml from charts directory. Closes #3321 Jan 31, 2018
@splisson splisson changed the title fix(helm): Accept dependency in requirements.yaml from charts directory. Closes #3321 fix(helm): Accept dependency in requirements.yaml from charts directory. Closes #3221 Jan 31, 2018
@bacongobbler bacongobbler changed the title fix(helm): Accept dependency in requirements.yaml from charts directory. Closes #3221 fix(helm): Accept dependency in requirements.yaml from charts directory Feb 27, 2018
@bacongobbler
Copy link
Member

Hey @splisson, thanks again for the PR. I noticed there are a ton of changes being made to existing function signatures but no unit tests to cover these cases. Would you mind writing these such that we can ensure the code works the way it should? Thanks!

@splisson
Copy link
Contributor Author

Sure

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@sgandon
Copy link

sgandon commented Jun 28, 2018

I wonder if this is not similar to this PR that I made a while ago.
#3987

@YingjunHu
Copy link

@sgandon Any updates on this PR? Is the problem fixed? I'm still seeing the problem in 2019. I'd appreciate if you can still see this comment..

@YingjunHu YingjunHu mentioned this pull request Mar 29, 2019
Closed
@iulianbaranai iulianbaranai mentioned this pull request Jul 5, 2019
Closed
@sgandon
Copy link

sgandon commented Jul 8, 2019

It too one year for someone to review my PR #3987, and a few days to fixed proposed changes. Let's hope it does not take another year to have it merged.

@YingjunHu
Copy link

It's pretty frustrated to see this happen.. Although I've also found the way to work around it, it would be great if they can merge it... Been tracing this issue for half a year already. Thanks @sgandon.
Happy helming?? Lol

@bacongobbler
Copy link
Member

bacongobbler commented Sep 30, 2019 •
edited
Loading

Hi @splisson, sorry for leaving this one dangling for so long. This PR seems much cleaner than the proposed solution in #3987. If you would be willing to rebase this PR, that would be great. If not, please let us know and someone else can carry this PR forwards. Thanks!

@helm-bot helm-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 30, 2019
@bacongobbler bacongobbler mentioned this pull request Sep 30, 2019
Closed
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 30, 2019
Till Hoffmann and others added 3 commits September 30, 2019 13:39
@splisson
fixed typo
bea76b6 
Signed-off-by: Till Hoffmann <till.hoffmann@enforge.de>
Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@ammarn911 @splisson
Adding MicroK8s to kubernetes_distros.md
fca5c6b 
Signed-off-by: ammarn911 <49419471+ammarn911@users.noreply.github.com>
Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@hypnoglow @splisson
ref(helm): expose Get for repository file
db578b5 
Get method is a useful shortcut that can be used
by other apps that use helm repo package.

Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@splisson
helm#3321 Accept dependency in requirements.yaml from charts directory.
ef75b6b 
Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@splisson
Added test cases for chart present under charts folder
74d4bed 
Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@splisson
missing test file
07ed802 
Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@splisson
Removed test requiring repository field to allow local chart dependency
c8506ac 
Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@bacongobbler
Copy link
Member

it looks like a few stray commits snuck in here (like fca5c6b). Let me know once this has been fixed up and I'll be happy to review again.

@splisson
Copy link
Contributor Author

splisson commented Oct 2, 2019 via email •
edited
Loading

@splisson
Revert "ref(helm): expose Get for repository file"
b8f17ee 
This reverts commit db578b5.

Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@splisson
Revert "Adding MicroK8s to kubernetes_distros.md"
89da0d5 
This reverts commit fca5c6b.

Signed-off-by: Sebastien Plisson <sebastien.plisson@gmail.com>
@bacongobbler bacongobbler added this to the 2.15.0 milestone Oct 2, 2019
@bacongobbler
Copy link
Member

bacongobbler commented Oct 3, 2019 •
edited
Loading

Sorry @splisson, but a git revert is slightly different than removing the commit from the history. We need the commit removed from the branch's history entirely, or else the commit history on master will show the same commit merged twice, then reverted once.

What I suggest you could do is run a git rebase --interactive master. It will open a new window which you can remove commits from the branch history entirely by removing them from the timeline.

More info here: https://thoughtbot.com/blog/git-interactive-rebase-squash-amend-rewriting-history#interactive-rebase

@bacongobbler
Copy link
Member

fixed up in #6578. Thanks @splisson! :)

@bacongobbler bacongobbler mentioned this pull request Oct 3, 2019
Merged
@splisson
Copy link
Contributor Author

splisson commented Oct 3, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. feature size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
2.15.0
Development

Successfully merging this pull request may close these issues.

Helm should allow easier "management" of child charts placed manually in charts
10 participants
@splisson @jascott1 @bacongobbler @fejta-bot @sgandon @YingjunHu @k8s-ci-robot @helm-bot @ammarn911 @hypnoglow