2
\$\begingroup\$

I use the following code which works ok!

The code is installing helm charts in loop in k8s cluster, and I want to verify that I'm writing it ok.

tmpfile, err := ioutil.TempFile(kp, kcp)
if err != nil {
    log.Error(err, "error")
}

defer os.Remove(tmpfile.Name()) 

if _, err := tmpfile.Write(cfg); err != nil {
    return err
}
if err := tmpfile.Close(); err != nil {
    return err
}

kcfgFilePath := tmpfile.Name()
settings := cli.New()
ac := new(action.Configuration)
clientGetter := genericclioptions.NewConfigFlags(false)
clientGetter.KubeConfig = &kcfgFilePath



for _, chartInstallation := range charts {
    chart, err := loader.Load(chartInstallation.Path)
    if err != nil {
        return err
    }

    releaseName := releaseName + "-" + chartInstallation.Name
    if err := ac.Init(clientGetter, settings.Namespace(), os.Getenv("HELM_DRIVER"), func(format string, v ...interface{}) {
        
    }); err != nil {
        return err
    }
    releasePresent := true
    statusAction := action.NewStatus(ac)
    status, err := statusAction.Run(releaseName)
    if err != nil {
        if strings.Contains(err.Error(), driver.ErrReleaseNotFound.Error()) {
            releasePresent = false
        } else {
            return err
        }
    }

    if !releasePresent {
        // install chart
        installAction := action.NewInstall(ac)
        installAction.CreateNamespace = true
        installAction.Namespace = chartInstallation.Namespace
        installAction.ReleaseName = releaseName

        _, err := installAction.Run(chart, nil)
        if err != nil {
            return err
        }
        log.Info("chart installed: ", "releaseName", releaseName)
    }

    if status != nil {
        if releasePresent && status.Info.Status.String() == release.StatusFailed.String() { 
            upgradeAction := action.NewUpgrade(ac)
            upgradeAction.Wait = true
            upgradeAction.ReuseValues = false
            upgradeAction.Recreate = false
            _, err := upgradeAction.Run(releaseName, chart, nil)
            if err != nil {
                return err
            }
        }
    }
\$\endgroup\$
2
\$\begingroup\$
  • The error from ioutil.TempFile is unhandled, should also return err since everything after it won't work if the temporary file couldn't be created.
  • The strings.Contains check is definitely the least preferred option to check for the kind of error. If at all possible avoid it since it's actually fairly easy for that string to change with, say, a library upgrade without any notice, which would break the logic here.
  • Same with status.Info.Status.String() == release.StatusFailed - are you sure it can't just be compared without the String() conversion?
  • tmpfile.Name() is used twice, could already be moved to a variable.
  • In fact, one of tmpfile and kcfgFilePath is redundant, I'd suggest directly assigning to kcfgFilePath.
  • IMO new is obsolete and could very well be replaced by a regular variable declaration.
  • statusAction could be inlined.

Actually it would be great if the imports were mentioned, that would give readers a chance to find the actual signatures and such. Without that I'll leave it at those points.

Apart from these details it looks okay, though at some point you might want to refactor things into smaller functions to bring a little order into it. Right now it's still a rather long block of code. E.g. it might be worth moving the inner part of the loop into a new function. Or creating functions for each "action" that's currently only marked with a comment, say runInstall, runUpgrade. The temporary file bit at the start could also very well be its own function (keeping the os.Remove call at this level of course).

Edit: As requested to elaborate on the second point, it's better to do a direct comparison of error values instead of string comparisons, since they're just values too, this will be faster and not depend on string values which might change in unexpected ways, like so:

// this is defined in the helm API
var ErrReleaseNotFound error = errors.New("release: not found")

// ...

// some time later, we get an error
{
    statusAction := action.NewStatus(ac)
    status, err := statusAction.Run(releaseName)
    if err != nil {
        if err == driver.ErrReleaseNotFound {
            releasePresent = false
        } else {
            return err
        }
    }

    // ...
}

But, actually, we should all probably start using errors.Is so that even nested errors would be correctly handled:

// ...
if errors.Is(err, ErrReleaseNotFound) {
    releasePresent = false
} else {
    return err
}
// ...
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Thanks a lot, 1+ , one remark, could you please provide an example for the second option ?strings.Contains how its better to use it? \$\endgroup\$ Mar 10 at 13:44
  • \$\begingroup\$ @bredstraruts added some, hope that helps. \$\endgroup\$
    – ferada
    Mar 11 at 9:54
  • 1
    \$\begingroup\$ Thanks I learn a lot from you! \$\endgroup\$ Mar 11 at 12:14
  • \$\begingroup\$ HI again , i've another question in that domain could you please have a look? thanks!codereview.stackexchange.com/questions/259652/… \$\endgroup\$ Apr 17 at 6:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.