commands/.../new,pkg/scaffold/helm: source chart flag for new projects #949
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…art when creating new helm projects
openshift-ci-robot
added
the
size/L
…r helm CLI, add helm-chart-version flag, general cleanup
joelanford
changed the title
joelanford
requested review from
estroz,
lilic,
hasbro17,
shawn-hurley and
AlexNPavel
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
added
needs-rebase
lilic
reviewed
Feb 1, 2019
openshift-ci-robot
removed
the
needs-rebase
…ic with --helm-chart
…atures and add CreateChart godoc comment
…phens in scaffolded chart name
joelanford
force-pushed
the
helm-new-source-chart
branch
from
dddd92d to
b7c93b9
Compare
|
@joelanford We can probably mention this feature in the CHANGELOG. Also the godocs for |
|
@hasbro17 Thanks! I'll go ahead and update the CLI doc so I can reference it in the CHANGELOG. |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
added
size/XXL
lilic
reviewed
Feb 18, 2019
| return s.Input, nil | ||
| } | ||
|
|
||
| const watchesYAMLTmpl = `--- | ||
| - version: {{.Resource.Version}} | ||
| group: {{.Resource.FullGroup}} | ||
| kind: {{.Resource.Kind}} | ||
| chart: /opt/helm/{{.HelmChartsDir}}/{{.Resource.LowerKind}} | ||
| chart: /opt/helm/{{.HelmChartsDir}}/{{.ChartName}} |
There was a problem hiding this comment.
Quick question: Can ChartName be allowed to contain uppercase chars? If yes, should we convert it to lowercase here?
There was a problem hiding this comment.
According to the chart name conventions, chart names SHOULD be lower case. However, the directory name MUST match the name in Chart.yaml. It seems the chart name case is not enforced, but the directory matching the name in Chart.yaml is.
$ helm create Test
Creating Test
$ helm package Test
Successfully packaged chart and saved it to: /tmp/Test-0.1.0.tgz
$ mv Test test
$ helm package test
Error: directory name (test) and Chart.yaml name (Test) must matchSince our code works by using Helm's packages to load the chart metadata, .ChartName is effectively a read-only field provided by Helm. For that reason, I think I lean toward accepting what Helm gives us without modification. I would expect Helm to error out if a user tried to create or fetch an invalid chart.
lilic
pushed a commit
to lilic/operator-sdk
that referenced
this pull request
Feb 27, 2019
operator-framework#949) * commands/operator-sdk/cmd/new,pkg/scaffold/helm: allow source helm chart when creating new helm projects * pkg/scaffold/helm/chart.go,commands/.../new.go: remove requirement for helm CLI, add helm-chart-version flag, general cleanup * commands/operator-sdk/cmd/new.go: improve helm-chart flags' help texts * commands/operator-sdk/cmd/new.go: comment to describe verifyFlags logic with --helm-chart * pkg/scaffold/helm/chart.go: revert to using r.LowerKind for scaffolded chart name * commands/.../new.go,pkg/scaffold/helm/chart.go: improve function signatures and add CreateChart godoc comment * pkg/scaffold/helm/chart.go: comment to explain decision not to use hyphens in scaffolded chart name * commands/operator-sdk/cmd/new.go: fix typo in help text * commands/operator-sdk/cmd/new.go: fix helm verify flags logic * pkg/scaffold/helm/chart.go: log tmpDir remove failure * CHANGELOG.md,doc,pkg/scaffold/helm: update docs * vendor: add necessary vendor dirs * CHANGELOG.md: moving updates to Unreleased section * Gopkg.lock,vendor: removing non-go
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change:
This PR adds new flags to the
operator-sdk newcommand that enable new Helm projects to be created from existing Helm charts.--helm-chart- The chart to use when creating the project. Format is:<url><repo>/<chart_name><chart_name>(with--helm-chart-repospecified)/path/to/chart.tgz(local chart archive)/path/to/chartDir(local chart directory)--helm-chart-version- The version of the chart to fetch from a remote repository.--helm-chart-repo- The URL of the Helm chart repo in which to find the chart specified by--helm-chartIt also makes
--api-versionand--kindoptional if--helm-chartis set, If left unset,--api-versionis defaulted tocharts.helm.k8s.io/v1alpha1and--kindis deduced from the fetched chart.Examples:
From a URL:
From stable repo:
From custom repo:
From local archive:
From a local directory:
Motivation for the change:
At the suggestion of @robszumski, I put together this PR to simplify the UX for building operators from existing Helm charts.
NOTE: This PR exposes only some of the flags that
helm fetchdoes, so not all use cases are supported (e.g. users will not be able to authenticate to a remote repo using the operator-sdk CLI from this PR).