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

fix(helm): resolve URLs and SemVers correctly #1292

Merged
merged 1 commit into from
Oct 6, 2016

Conversation

technosophos
Copy link
Member

@technosophos technosophos commented Oct 6, 2016

The original dependency resolution did not correctly resolve version or
URL of a dependency. Version was tracked by filename, and URL was
assumed to be absolute. This fixes both of those.

Closes #1277


This change is Reviewable

The original dependency resolution did not correctly resolve version or
URL of a dependency. Version was tracked by filename, and URL was
assumed to be absolute. This fixes both of those.

Closes helm#1277
@jackfrancis
Copy link

LGTM pending CI

@technosophos technosophos merged commit 7b222ce into helm:master Oct 6, 2016
return normalizeURL(repourl, verEntry.URLs[0])
}

return normalizeURL(repourl, verEntry.URLs[0])
Copy link
Member

Choose a reason for hiding this comment

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

how many levels deep are we willing to go to find a chart?

Copy link
Member Author

Choose a reason for hiding this comment

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

ALL the way down.

I'll see if I can break that up in a related branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, that change is in #1293, which is directly related to this anyway.

"testing"

"k8s.io/helm/cmd/helm/helmpath"
//"k8s.io/helm/pkg/repo"
Copy link
Member

Choose a reason for hiding this comment

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

ewww, commented out package

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll kill that one on a related branch. Good catch.

@technosophos technosophos deleted the fix/1227-broken-helm-dep-up branch October 28, 2016 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: chart dependencies requirements.yaml mentioned in developer.md do not work
4 participants