🌱 Helm extension prototype

[skattoju]

Description

Building on the initial prototype that adds functionality to install helm charts as extensions, additional functionality has been added to allow for the specification of values in the form of config maps, secrets or plain text.

Reviewer Checklist

• API Go Documentation
• Tests: Unit Tests (and E2E Tests, if appropriate)
• Comprehensive Commit Messages
• Links to related GitHub Issue(s)

[everettraven]

What if you did something like a discriminated union here?

I'm thinking something like:

type ConfigSourceType string

const (
  ConfigSourceTypeConfigMap = "ConfigMap"
  ConfigSourceTypeSecret = "Secret"
  ConfigSourceTypeInline = "Inline" 
)

// CEL validation here to ensure type + property correctness
type ConfigSource struct {
  // +required
  Type ConfigSourceType `json:"type"`
  // +optional
  ConfigMap *ConfigMapReference `json:"configMap,omitempty"`
  ...
}

Then line 149 could become:

ConfigSources []ConfigSource

I think something like this might fit better with the existing conventions of the ClusterExtension API

[everettraven]

This function is generally pretty straightforward but I wonder if the mental model here could be simplified a bit with some logical abstractions or separate functions?

I think the discriminated union approach for the API might also make that abstraction/functional separation a bit nicer.

Maybe something like:

func processConfigSources(ctx context.Context, client *kubernetes.Clientset, configSources []ocv1alpha1.ConfigSource, values chartutil.Values) (chartutil.Values, error) {
  for _, configSource := range configSources {
    userSuppliedValues, err := loadValuesFromConfigSource(ctx, client, configSource)
    // err check
    // do something with values
  }
  // return loaded values
}

func loadValuesFromConfigSource(ctx context.Context, client *kubernetes.Clientset, configSource ocv1alpha1.ConfigSource) (chartutil.Values, error) {
  switch configSource.Type {
    case ocv1alpha1.ConfigSourceTypeConfigMap:
      // Load from configmap
    case ocv1alpha1.ConfigSourceTypeSecret:
      // Load from secret
    case ocv1alpha1.ConfigSourceTypeInline:
      // Load from inline
  }
}

What I put might not work as is, but hopefully gets the point across that I think some responsibility separation here might be useful for readability, maintenance, and testing.