Publish to helm registry #91

sherry-ummen · 2020-05-12T22:35:44Z

Support for publish to Helm Registry

Fixes #90

Its using method defined here : https://helm.sh/docs/topics/registries/

Basically following command:

export HELM_EXPERIMENTAL_OCI=1
helm registry login -u myuser localhost:5000
helm registry logout localhost:5000
helm chart save mychart/ localhost:5000/myrepo/mychart:2.7.0
helm chart push localhost:5000/myrepo/mychart:2.7.0

Tested it with Azure Container Registry

ErinCall

Despite all my quibbles, this looks great overall. Thanks for diving into it!

ErinCall · 2020-05-22T19:22:29Z

docs/parameter_reference.md

+| Param name             | Type     | Required | Alias                  | Purpose |
+|------------------------|----------|----------|------------------------|---------|
+| registry_url           | string   | yes      |                        | Registry url to where the chart be published|
+| registry_login_user_id | string   | yes      |              | Login ID for the Helm registry |
+| registry_login_password| string   | yes      |        | Login Password for the Helm registry |
+| registry_repo_name     | string   | yes      |        | Repository name in the Helm registry |
+| chart_version       | string   |   yes  |  | Version of the chart |
+| chart                  | string   |          |                        | Local path of the chart |


Would you mind aligning the whitespace here? It doesn't matter for the rendered markdown, but it makes the file easier to read while editing :)

Suggested change

| Param name | Type | Required | Alias | Purpose |

|------------------------|----------|----------|------------------------|---------|

| registry_url | string | yes | | Registry url to where the chart be published|

| registry_login_user_id | string | yes | | Login ID for the Helm registry |

| registry_login_password| string | yes | | Login Password for the Helm registry |

| registry_repo_name | string | yes | | Repository name in the Helm registry |

| chart_version | string | yes | | Version of the chart |

| chart | string | | | Local path of the chart |

| Param name | Type | Required | Alias | Purpose |

| ------------------------ | ---------- | ---------- | ------------------------ | --------- |

| registry_url | string | yes | | Registry url to where the chart be published |

| registry_login_user_id | string | yes | | Login ID for the Helm registry |

| registry_login_password | string | yes | | Login Password for the Helm registry |

| registry_repo_name | string | yes | | Repository name in the Helm registry |

| chart_version | string | yes | | Version of the chart |

| chart | string | | | Local path of the chart |

ErinCall · 2020-05-22T19:30:35Z

internal/run/chart.go

+
+	if reg.subCommand == "save" {
+		args = append(args, "save")
+		args = append(args, reg.chartPath)


Does this need to check for an empty chartPath? I haven't checked how helm save "" [ref] behaves, but I wouldn't expect it to be the same as helm save [ref].

Suggested change

args = append(args, reg.chartPath)

if reg.chartPath != "" {

args = append(args, reg.chartPath)

}

ErinCall · 2020-05-22T19:35:48Z

internal/run/chart.go

+		cmd := fmt.Sprintf("%s/%s:%s", reg.registryURL, reg.registryRepoName, reg.chartVersion)
+		args = append(args, cmd)


These two lines are the same as their counterparts in reg.subCommand == "save", right? If so they should go after the if clause, so they're less likely to accidentally diverge in the future.

ErinCall · 2020-05-22T19:44:42Z

internal/env/config.go

+	RegistryURL           string   `envconfig:"registry_url"`
+	RegistryLoginUserID   string   `envconfig:"registry_login_user_id"`
+	RegistryLoginPassword string   `envconfig:"registry_login_password"`
+	RegistryRepoName      string   `envconfig:"registry_repo_name"`


Newly-added settings don't need entries in settingAliases—the aliases are for backwards compatibility if a setting gets renamed.

ErinCall · 2020-05-22T19:46:59Z

internal/helm/plan.go

@@ -136,3 +139,16 @@ var lint = func(cfg env.Config) []Step {
 var help = func(cfg env.Config) []Step {
 	return []Step{run.NewHelp(cfg)}
 }
+
+var publish = func(cfg env.Config) []Step {
+	fmt.Println("inside publish")


There's some leftover debug logging here

Suggested change

fmt.Println("inside publish")

ErinCall · 2020-05-22T20:10:02Z

internal/run/chart.go

+func (reg *Chart) Execute() error {
+	return reg.cmd.Run()


Minor: there's a bit of copy-paste left over here (and the same thing in Prepare)

Suggested change

func (reg *Chart) Execute() error {

return reg.cmd.Run()

func (chart *Chart) Execute() error {

return chart.cmd.Run()

ErinCall · 2020-05-22T20:16:31Z

internal/run/registry.go

+}
+
+// Prepare gets the Registry ready to execute.
+func (reg *Registry) Prepare() error {


Same as above—this should check for missing config, and a test for these cases would be nice if you have the time.

Suggested change

func (reg *Registry) Prepare() error {

func (reg *Registry) Prepare() error {

if reg.userID == "" {

return fmt.Errorf("registry_login_user_id is required")

}

if reg.password == "" {

return fmt.Errorf("registry_login_password is required")

}

if reg.registryURL == "" {

return fmt.Errorf("registry_url is required")

}

ErinCall · 2020-05-22T20:32:00Z

internal/helm/plan.go

+	fmt.Println("inside publish")
+	var steps []Step
+
+	os.Setenv("HELM_EXPERIMENTAL_OCI", "1")


Rather than a bare os.Setenv, please set this in the environment for the cmd in Chart and Registry:

environ := os.Environ() environ = append(environ, "HELM_EXPERIMENTAL_OCI=1") reg.cmd.Env(environ)

Changing the environment mid-execution doesn't always cause problems, but when problems do arise it's a headache trying to track them down.

ErinCall · 2020-05-22T20:35:02Z

internal/run/registry_test.go

+		RegistryURL:           "registry_url",
+	}
+
+	u := NewRegistry("login", cfg)


Minor: some leftover copy-paste here (and in TestPrepareAndExecuteLogout).

Suggested change

u := NewRegistry("login", cfg)

reg := NewRegistry("login", cfg)

ErinCall · 2020-05-22T21:15:04Z

README.md

+      chart: ./
+    environment:
+      REGISTRY_URL: 
+        from_secret: registry_url
+      REGISTRY_LOGIN_USER_ID: 
+        from_secret: registry_login_user_id
+      REGISTRY_LOGIN_PASSWORD:
+        from_secret: registry_login_password
+      REGISTRY_REPO_NAME: <helm_repository_name>
+      CHART_VERSION: <version>


A couple of nitpicks here:

Since registry_repo_name and chart_version don't need to come from secrets, the settings block is a more appropriate spot for them.

The README examples should only contain the required params, to minimize the amount of scrolling people have to do.

Suggested change

chart: ./

environment:

REGISTRY_URL:

from_secret: registry_url

REGISTRY_LOGIN_USER_ID:

from_secret: registry_login_user_id

REGISTRY_LOGIN_PASSWORD:

from_secret: registry_login_password

REGISTRY_REPO_NAME: <helm_repository_name>

CHART_VERSION: <version>

registry_repo_name: <helm_repository_name>

chart_version: <version>

environment:

REGISTRY_URL:

from_secret: registry_url

REGISTRY_LOGIN_USER_ID:

from_secret: registry_login_user_id

REGISTRY_LOGIN_PASSWORD:

from_secret: registry_login_password

sherry-ummen · 2020-05-27T19:41:32Z

Hey thanks for the review, I will fix these soon. sorry for the delay

josmo · 2022-08-23T16:41:56Z

This is pushing a couple years so I'm going to close this out - if issues get address we can reopen :)

sherry-ummen added 4 commits May 12, 2020 22:25

registry command, config added and tests

3b1ab19

chart push and save command with tests

356bdaa

binary updated

14532d1

documentation updated

936e8c1

sherry-ummen requested review from ErinCall, grinnellian, josmo and kav as code owners May 12, 2020 22:35

ErinCall suggested changes May 22, 2020

View reviewed changes

ErinCall mentioned this pull request May 22, 2020

Add a comment explaining the purpose env.settingAliases #95

Closed

josmo closed this Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publish to helm registry #91

Publish to helm registry #91

sherry-ummen commented May 12, 2020 •

edited by ErinCall

Loading

ErinCall left a comment

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

ErinCall May 22, 2020

sherry-ummen commented May 27, 2020

josmo commented Aug 23, 2022

		cmd := fmt.Sprintf("%s/%s:%s", reg.registryURL, reg.registryRepoName, reg.chartVersion)
		args = append(args, cmd)

-func (reg *Registry) Prepare() error {
+func (reg *Registry) Prepare() error {
+	if reg.userID == "" {
+		return fmt.Errorf("registry_login_user_id is required")
+	}
+	if reg.password == "" {
+		return fmt.Errorf("registry_login_password is required")
+	}
+	if reg.registryURL == "" {
+		return fmt.Errorf("registry_url is required")
+	}

	u := NewRegistry("login", cfg)
	reg := NewRegistry("login", cfg)

Publish to helm registry #91

Publish to helm registry #91

Conversation

sherry-ummen commented May 12, 2020 • edited by ErinCall Loading

ErinCall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherry-ummen commented May 27, 2020

josmo commented Aug 23, 2022

sherry-ummen commented May 12, 2020 •

edited by ErinCall

Loading