3
\$\begingroup\$

Im fairly new to Golang and I appreciate any feedback about how to make the code cleaner.

The code is called by go-routine and should update status (fail/success etc) of several installation process.

package avss

import (
    "context"
    "sync"

    "helm.sh/helm/v3/pkg/release"

    "sigs.k8s.io/controller-runtime/pkg/client"

    avmv1alpha1 "github.vmp/avs/api/v1alpha1"
)

func GetUpdateStatus(ctx context.Context, log logr.Logger, cr client.Object, status *avmv1alpha1.ComponentStatusWithAggregation, statusClient client.StatusClient) func(chartStatus *avmv1alpha1.InstallStatus) error {
    var conditionMutex sync.Mutex

    return func(chartStatus *avmv1alpha1.InstallStatus) error {

        // prevent status update by subroutines at the same time, lock the access to prevent concurrent modification
        conditionMutex.Lock()
        defer conditionMutex.Unlock()
        status.ChartStatus = updateStatus(chartStatus, status.ChartStatus)
        status.Status = overallStatus(status.ChartStatus)
        err := statusClient.Status().Update(ctx, cr)
        return err
    }
}

func overallStatus(chartsStatus []avmv1alpha1.InstallStatus) string {
    overallStatus := ""
    for _, status := range chartsStatus {
        switch status.ReleaseStatus {
        case release.StatusFailed.String():
            overallStatus = "error"
        case release.StatusDeployed.String():
            if overallStatus == "" {
                overallStatus = "installed"
            }
        default:
            if overallStatus != release.StatusFailed.String() {
                overallStatus = "reconcile"
            }
        }
    }
    return overallStatus
}

func updateStatus(newStatus *avmv1alpha1.ChartStatus, cStatus []avmv1alpha1.InstallStatus) []avmv1alpha1.ChartStatus {
    found := false
    for i, status := range cStatus {
        if status.Name == newStatus.Name {
            found = true
            cStatus[i] = *newStatus
        }
    }
    if !found {
        cStatus = append(cStatus, *newStatus)
    }
    return cStatus
}

https://play.golang.org/p/9C8xGV2116O

The code is working! however, I guess it can be improved as this is my first Go program which should be run in production :-)

If the code is perfect, 🤩 please let me know

\$\endgroup\$
1
  • \$\begingroup\$ I think it is proper.. changes that could be made are ("installed" or "reconcile" -> if this value is being used elsewhere I would suggest you define them as constants in a common go file like params.go) \$\endgroup\$ – Siva Guru May 20 at 13:43
0
\$\begingroup\$

Overall this looks like a pretty sensible file. Some minor points you might want to consider follow.

Imports

logr is not imported in the file. I presume this is https://github.com/go-logr/logr. It's passed into the first function but not used. Perhaps remove it from the signature?

Discrete strings

overallStatus returns one of four strings. You could define these strings as constants like Siva mentioned. A step further is to introduce a type that groups these strings together. This

  • ensures the function returns one of the strings defined, and
  • makes it clear what strings can be returned from the function by looking at the signature.
type status string

const (
    statusUnknown   status = ""
    statusError     status = "error"
    statusInstalled status = "installed"
    statusReconcile status = "reconcile"
)
\$\endgroup\$

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.