Closed vdice closed 4 years ago
Hi, I did some work on my end regarding this capability but hadn't the chance to test it. Please let me know if this is the right direction:
Repositories map[string]RepositoryArguments `yaml:"repos"`
type RepositoryArguments struct {
URL string `yaml:"url"`
Cafile string `yaml:"cafile"`
Certfile string `yaml:"certfile"`
Keyfile string `yaml:"keyfile"`
Username string `yaml:"username"`
Password string `yaml:"password"`
}
cmdRepos := m.NewCommand("helm3")
for name, repo := range step.Repositories {
AddRepository(cmdRepos, name, repo.URL, repo.Cafile, repo.Certfile, repo.Keyfile, repo.Username, repo.Password)
}
Create repo
func AddRepository(cmdAddRepo *exec.Cmd, name, url, cafile, certfile, keyfile, username, password string) error {
if name == "" && url != "" {
return fmt.Errorf("empty field name")
}
cmdAddRepo.Args = append(cmdAddRepo.Args, "repo", "add", name, url)
if certfile != "" && keyfile != "" {
cmdAddRepo.Args = append(cmdAddRepo.Args, "--cert-file", certfile, "--key-file", keyfile)
}
if cafile != "" {
cmdAddRepo.Args = append(cmdAddRepo.Args, "--ca-file", cafile)
}
if username != "" && password != "" {
cmdAddRepo.Args = append(cmdAddRepo.Args, "--username", username, "--password", password)
}
fmt.Sprintf("Adding repo %v %v", name, url)
err := cmdAddRepo.Run()
if err != nil {
return errors.Wrapf(err, "unable to add the requested repository '%s'", name)
}
return err
}
@MChorfa excellent! The RepositoryArguments
config looks great. The other code looks great as well, but I wanted to mention the following:
Whilst the original feature request indeed mentioned consuming this information at runtime in the form of an action step (install, upgrade, etc.), we'd since thought about it and decided that consuming the information at build time (Dockerfile generation) may be preferable. Then, any action step can simply start using the added charts/chart repos and one doesn't need to add the same repository config for each action.
(That being said, it might be useful to have both: the ability to specify at the mixin level and on an individual action step. Thoughts @carolynvs ?)
For implementing the latter approach of mixin-level/build-time support, there currently exists a model in the az
mixin. Check out the build logic here: https://github.com/deislabs/porter-az/blob/master/pkg/az/build.go
So, we could use the structure you have above to parse the config and then iterate through the repos to add the appropriate RUN helm repo add ...
statements to the Dockerfile lines.
Thanks a lot for the work here!
Hello @vdice, thank you for the feedback. Yes, it would be nice to have the mixin-level/build-time support
and I would suggest adding stable
repo as a part of helm installation at build time.
fmt.Fprintln(m.Out, `RUN helm3 repo add stable https://kubernetes-charts.storage.googleapis.com && helm3 repo update`)
Thank you
I think having both is fine however... we want to strongly encourage building it into the bundle, instead of doing it on the fly. We plan on having a linter run during porter build
to check for things like helm repo add
and warn people to not do it, and link to a best practice guide. So we should prioritize implementing the mixin config first.
As a user of this mixin needing to fetch charts from chart repos, I would like a way to specify which chart repos should be fetched at build-time via mixin config. Something like this in a
porter.yaml
:Inspired by https://github.com/deislabs/porter-helm/issues/8#issuecomment-473788262, the current thinking is that a repo entry may contain additional fields in the future (supporting auth credentials or other), but for this first iteration, the single
url
property seems sufficient.This mixin would then be in charge of parsing the configuration and adding the relevant repo add commands into the build steps (Dockerfile lines), e.g.
helm repo add brigade https://brigadecore.github.io/charts
, etc.There is prior art in consuming such extra mixin configuration; see the az mixin.
This ticket supersedes the original issue/design in https://github.com/deislabs/porter-helm/issues/8