Lokoctl uses global variable which gives wrong results if used via APIs

[surajssd]

Consider following test code:

func TestConversion(t *testing.T) {
    testCases := []struct {
        Name        string
        InputConfig string
        Contains    string
    }{
        {
            Name:     "use external_url param",
            Contains: "https://prometheus.externalurl.net",
            InputConfig: `
component "prometheus-operator" {
  prometheus {
    external_url = "https://prometheus.externalurl.net"
  }
}
`,
        },
        {
            Name:     "no external_url param",
            Contains: "https://prometheus.mydomain.net",
            InputConfig: `
component "prometheus-operator" {
  prometheus {
    ingress {
      host                       = "prometheus.mydomain.net"
      class                      = "contour"
      certmanager_cluster_issuer = "letsencrypt-production"
    }
  }
}
`,
        },
    }

    for _, tc := range testCases {
        tc := tc
        t.Run(tc.Name, func(t *testing.T) {
            m := renderManifests(t, tc.InputConfig)
            gotConfig := m["prometheus-operator/templates/prometheus/prometheus.yaml"]

            scanner := bufio.NewScanner(strings.NewReader(gotConfig))
            for scanner.Scan() {
                line := scanner.Text()
                if !strings.Contains(line, "externalUrl") {
                    continue
                }

                t.Logf("\nwanted =   externalUrl: %q.\ngot    = %s", tc.Contains, line)
            }
        })
    }
}

Whose output looks like this:

$ go test -v github.com/kinvolk/lokomotive/pkg/components/prometheus-operator -run TestConversion
=== RUN   TestConversion
=== RUN   TestConversion/use_external_url_param
    component_conversion_test.go:92: 
        wanted =   externalUrl: "https://prometheus.externalurl.net".
        got    =   externalUrl: "https://prometheus.externalurl.net"
=== RUN   TestConversion/no_external_url_param
    component_conversion_test.go:92: 
        wanted =   externalUrl: "https://prometheus.mydomain.net".
        got    =   externalUrl: "https://prometheus.externalurl.net"
--- PASS: TestConversion (1.86s)
    --- PASS: TestConversion/use_external_url_param (0.89s)
    --- PASS: TestConversion/no_external_url_param (0.97s)
PASS
ok      github.com/kinvolk/lokomotive/pkg/components/prometheus-operator        1.906s

Notice that in second test case we should see https://prometheus.mydomain.net but the old result persists for some reason. My first guess is that this is due to use of globals.

I think component.LoadConfig and component.RenderManifests don't do the right thing. Or we should flush out old values when components.Get is called.

func renderManifests(t *testing.T, configHCL string) map[string]string {
    name := "prometheus-operator"

    component, err := components.Get(name)
    if err != nil {
        t.Fatalf("Getting component %q: %v", name, err)
    }

    body, diagnostics := util.GetComponentBody(configHCL, name)
    if diagnostics != nil {
        t.Fatalf("Getting component body: %v", diagnostics)
    }

    diagnostics = component.LoadConfig(body, &hcl.EvalContext{})
    if diagnostics.HasErrors() {
        t.Fatalf("Valid config should not return an error, got: %s", diagnostics)
    }

    ret, err := component.RenderManifests()
    if err != nil {
        t.Fatalf("Rendering manifests with valid config should succeed, got: %s", err)
    }

    return ret
}

[invidian]

This is what https://github.com/kinvolk/lokomotive/pull/971 addresses.

Also related: #597.

[knrt10]

As this is already addressed PR of @invidian. Would you mind if this is assigned to you?

[invidian]

Okay, this again turns out to be never-ending story to do right:

• Right now we call function returning default configuration in each of the platform/component packages itself. The plan is to shift the calling to central platform/components packages, so function returning default configuration should become exporter.
• Exporting function returning default configuration without exporting configuration struct itself limits the usage (I can imagine one getting default config and zero-ing all fields on purpose manually).
• Exporting configuration struct builds up an API around it, exposing LoadConfig, Meta, Apply, Destroy, Initialize and PostApplyHook functions which can be misused when called on bad configuration. LoadConfig should be completely removed from it and Meta, Apply, Destroy, Initialize and PostApplyHook methods should be gated by a verifying function. Configuration struct should not implement Component/Platform interface. Validated configuration struct should.

Edit:

• Right now we call function returning default configuration in each of the platform/component packages itself. The plan is to shift the calling to central platform/components packages, so function returning default configuration should become exporter.

Platforms already have NewConfig() function exported, only aks platform didn't, so I think this can be used for now.

However, the problem still applies for components. I guess they could follow the same convention as platforms for now.

[invidian]

Created PR #1147 to definitely address this.