Create helm registry for ksonnet by bryanl · Pull Request #583 · ksonnet/ksonnet

bryanl · 2018-06-03T15:54:09Z

Creates a helm registry that can treat helm charts as ksonnet parts.

This change does the following:

If you have a helm registry defined in app.yaml, It will show up in the registry listing:

NAME        OVERRIDE PROTOCOL URI
====        ======== ======== ===
helm-stable          helm     https://kubernetes-charts.storage.googleapis.com

You can run ks pkg list and see charts in your helm repository

REGISTRY    NAME                           INSTALLED
========    ====                           =========
helm-stable acs-engine-autoscaler
helm-stable aerospike
helm-stable anchore-engine
helm-stable artifactory
helm-stable artifactory-ha
helm-stable aws-cluster-autoscaler
helm-stable bitcoind
helm-stable buildkite
helm-stable burrow

You can run ks pkg install to install a package:

$ ks pkg install helm-stable/acs-engine-autoscaler
INFO Retrieved 8 files

The pkg will then be vendored for you:

vendor/helm-stable/
└── acs-engine-autoscaler
    └── helm
        └── 2.2.0
            └── acs-engine-autoscaler
                ├── Chart.yaml
                ├── README.md
                ├── templates
                │   ├── NOTES.txt
                │   ├── _helpers.tpl
                │   ├── deployment.yaml
                │   └── secrets.yaml
                └── values.yaml

5 directories, 7 files

Signed-off-by: bryanl bryanliles@gmail.com

coveralls · 2018-06-03T15:59:39Z

Pull Request Test Coverage Report for Build 953

239 of 279 (85.66%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 69.734%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/cluster/apply.go	2	3	66.67%
pkg/cluster/cluster.go	0	4	0.0%
pkg/registry/manager.go	0	6	0.0%
pkg/helm/client.go	111	123	90.24%
pkg/registry/helm.go	91	108	84.26%

Totals
Change from base Build 950:	0.3%
Covered Lines:	9617
Relevant Lines:	13791

💛 - Coveralls

rimusz · 2018-06-03T16:55:30Z

👍

rimusz · 2018-06-03T16:58:59Z

would you mind to give comment access to the design doc, please?

underrun

just a couple questions... will dig in more Monday

underrun · 2018-06-04T00:44:53Z

Makefile

 VERSION?=dev-$(shell date +%FT%T%z)
 KS_BIN?=ks

-APIMACHINERY_VER := $(shell dep status | grep k8s.io/apimachinery | awk '{print $$3}')


is api machinery back/forward compat as of 1.8? i'm all for getting rid of this line as it requires someone to have run dep ensure before running make - just checking to make sure we can do this safely.

Another option here if we we want to keep the flexibility without the terrible build time:
APIMACHINERY_VER := $(shell grep -B1 k8s.io/apimachinery Gopkg.lock | head -n1 | cut -d'"' -f2)

underrun · 2018-06-04T01:27:06Z

pkg/registry/helm.go

+}
+
+// MakeRegistryRefSpec returns app registry ref spec.
+func (h *Helm) MakeRegistryRefSpec() *app.RegistryRefSpec {


why is this called MakeRegistryRefSpec instead of RegistryRefSpec?

shomron · 2018-06-04T14:50:28Z

pkg/registry/helm.go

+	spec             *app.RegistryRefSpec
+	repositoryClient HelmRepositoryClient
+	unarchiver       archive.Unarchiver
+	httpGet          httpGet


Why is this not encapsulated in the repositoryClient?

Because I missed it. It should be in there.

shomron · 2018-06-04T14:57:36Z

pkg/registry/helm.go

+		spec:             registryRef,
+		repositoryClient: rc,
+		unarchiver:       ua,
+		httpGet:          http.Get,


We should support request timeouts - http.DefaultClient does not.

shomron · 2018-06-04T14:59:41Z

pkg/registry/helm.go

+	Version     string   `json:"version,omitempty"`
+}
+
+type httpGet func(string) (*http.Response, error)


Is this style preferred over a getter interface?

shomron · 2018-06-04T15:05:32Z

pkg/registry/helm.go

+	"github.com/pkg/errors"
+)
+
+// Helm is a Helm repository.


Is there a good helm repository spec we could link to from somewhere? The most comprehensive I found was this.

The might be the best there is.

shomron · 2018-06-04T15:39:19Z

pkg/registry/helm.go

+	}
+
+	if hg == nil {
+		hg = http.Get


Same for timeouts.

shomron · 2018-06-04T15:41:13Z

pkg/registry/helm.go

+	}, nil
+}
+
+func (hrc *helmRepositoryClient) Repository() (*HelmRepository, error) {


Let's document exported methods.

This method isn't exported, but it should have documentation.

Fair enough, and agreed 😃

shomron · 2018-06-04T15:44:35Z

pkg/registry/helm.go

+		}
+	}()
+
+	b, err := ioutil.ReadAll(resp.Body)


How do we feel about reading arbitrarily long bodies into memory?

Not great, but this would required an onFile refactor. I noted this in #522.

shomron · 2018-06-04T15:46:19Z

pkg/registry/helm.go

+	defer func() {
+		cerr := resp.Body.Close()
+		if cerr != nil {
+			err = cerr


What's the intention here? What is generally done with errors within defer blocks?

This is an extremely hard error to trigger. If it does happen, return it instead of the initial error since it is most likely the cause of the problem. The reason it's a deferred function is because defer resp.Body.Close() will swallow the error.

But to pull that off correctly, would error need to be a named return value?

Also, if that's the plan it might be better to wrap any existing error rather than steam-rolling it.

shomron · 2018-06-04T16:07:17Z

pkg/registry/helm.go

+}
+
+func (hrc *helmRepositoryClient) Chart(name, version string) (*HelmRepositoryChart, error) {
+	repo, err := hrc.Repository()


This should probably be cached - do we want to download and parse multiple megabytes for each call?

I agree, going to add another issue for this.

shomron · 2018-06-04T16:17:16Z

pkg/registry/helm.go

+	}
+
+	if version == "" {
+		for _, chart := range repo.Latest() {


This seems like a lot of extra work - we're sorting all versions of all charts, then doing a linear search for the desired chart name. Why instead not sort just repo.Charts[name]?

shomron · 2018-06-04T16:58:23Z

pkg/registry/manager.go

 		return githubFactory(a, spec)
 	case ProtocolFilesystem:
 		return NewFs(a, spec)
+	case ProtocolHelm:


Is registry add support in scope for this PR?

Nope. Don't want to do that until we have a complete solution.

shomron · 2018-06-04T17:02:19Z

pkg/registry/manager.go

 	case ProtocolFilesystem:
 		return NewFs(a, spec)
+	case ProtocolHelm:
+		client, err := newHelmRepositoryClient("https://kubernetes-charts.storage.googleapis.com", nil)


Should this be configurable? Or at least const? Or will that be part of the registry add work?

It should be, but this is dependent on registry add. So for now, it is hard coded.

shomron · 2018-06-04T17:57:11Z

pkg/registry/helm.go

+		}
+
+		if resp.StatusCode != http.StatusOK {
+			return nil, nil, errors.Errorf("unexpected HTTP status code %q when retrieving helm chart", resp.Status)


Some more context in this error (chart? url?) would be useful - I'm not clear on whether those are added up the stack.

shomron · 2018-06-04T18:23:24Z

pkg/registry/helm.go

+			}
+		}()
+
+		handler := func(f *archive.File) error {


A comment along the lines of "file contents are returned to the caller via onFile handler would be useful to the uninitiated.

shomron · 2018-06-04T18:26:16Z

pkg/registry/helm.go

+
+			name := path.Join(chart.Name, "helm", chart.Version, f.Name)
+
+			return onFile(name, b)


I'd consider a future refactor of the ResolveFile interface to allow streaming with io.Reader all the way to the consumer.

Captured this change in #596

shomron · 2018-06-04T20:30:07Z

pkg/registry/helm.go

+
+	refSpec := &app.LibraryRefSpec{
+		Name:     partAlias,
+		Registry: h.Name(),


Conspicuously missing is GitVersion - should we discuss how we might better abstract away the version details while allowing them to be persisted in app.yaml?

There is no GitVersion if there is not GitRepo. This is an implementation detailed that leaked before registries were understood.

bryanl · 2018-06-07T16:46:07Z

@ksonnet/reviewers is this ready to go? Any more changes?

shomron · 2018-06-07T16:52:33Z

pkg/helm/client.go

+	Charts map[string][]RepositoryChart `json:"entries,omitempty"`
+}
+
+// FooLatest returns the latest version of charts in a Helm repository.


shomron · 2018-06-07T16:55:40Z

pkg/helm/client.go

+	Get(string) (*http.Response, error)
+}
+
+type getter struct {


I propose renaming to httpGetter - it will be clearer to read vs. the abstract getter interface.

shomron · 2018-06-07T16:59:30Z

pkg/helm/client.go

+
+// Repository returns the Helm repository's content.
+func (hrc *HTTPClient) Repository() (*Repository, error) {
+	resp, err := hrc.getter.Get(hrc.url.String())


Should we use Fetch() from below here?

shomron · 2018-06-07T16:59:48Z

pkg/helm/client.go

+}
+
+// Fetch fetches URLs from a repository. If uri is a path, it will use the client URL as the base.
+func (hrc *HTTPClient) Fetch(uri string) (io.ReadCloser, error) {


Should this be exported?

Yes, because the Helm registry uses it to resolve files and it is in another package.

shomron · 2018-06-07T18:07:58Z

Added a last round of comments. None need to be hard blockers.

Creates a helm registry that can treat helm charts as ksonnet parts. [design doc](https://docs.google.com/document/d/1FcK8pDqj_9EFjNVr9e_t9WemouQpS9ix-r0AxNDB_nM/edit?usp=sharing) Signed-off-by: bryanl bryanliles@gmail.com

bryanl self-assigned this Jun 3, 2018

bryanl requested review from shomron and underrun June 3, 2018 15:54

underrun reviewed Jun 4, 2018

View reviewed changes

shomron reviewed Jun 4, 2018

View reviewed changes

pkg/registry/helm.go Outdated

}

if hg == nil {

hg = http.Get

Copy link

Collaborator

shomron Jun 4, 2018

Choose a reason for hiding this comment

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

Same for timeouts.

shomron reviewed Jun 4, 2018

View reviewed changes

bryanl force-pushed the helm-registry branch 3 times, most recently from e6d54ca to c205f1e Compare June 7, 2018 16:43

shomron reviewed Jun 7, 2018

View reviewed changes

shomron approved these changes Jun 7, 2018

View reviewed changes

Create helm registry for ksonnet

f6da2d5

Creates a helm registry that can treat helm charts as ksonnet parts. [design doc](https://docs.google.com/document/d/1FcK8pDqj_9EFjNVr9e_t9WemouQpS9ix-r0AxNDB_nM/edit?usp=sharing) Signed-off-by: bryanl bryanliles@gmail.com

bryanl force-pushed the helm-registry branch from c205f1e to f6da2d5 Compare June 7, 2018 18:19

bryanl merged commit b03ceb4 into ksonnet:master Jun 7, 2018

bryanl deleted the helm-registry branch June 7, 2018 18:29


		name := path.Join(chart.Name, "helm", chart.Version, f.Name)

		return onFile(name, b)

Conversation

bryanl commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 953

💛 - Coveralls

Uh oh!

rimusz commented Jun 3, 2018

Uh oh!

rimusz commented Jun 3, 2018

Uh oh!

underrun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shomron Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanl commented Jun 3, 2018 •

edited

Loading

coveralls commented Jun 3, 2018 •

edited

Loading

shomron Jun 4, 2018 •

edited

Loading

bryanl Jun 4, 2018 •

edited

Loading