Don't use globals for handling components and platforms registration

[invidian]

The overhead of adding them is very little if we use the implementation, which do not use globals.

Similar to #596.

[johananl]

IMO we should rethink the whole notion of "registration". AFAICT the only advantage of the registration pattern is that when adding a new implementation you don't have to add it in some "central" location. The downsides are many though:

• Relying on multiple init() functions which is considered an anti-pattern in the Go community.
• Harder to follow the code compared to a "top-down" approach (i.e. constructing the right component in a central place).
• Side effects like race conditions.

That said, what alternative do you have in mind @invidian? The simplest approach is probably to have a slice of strings which represents all known platforms/components. This slice can then be used in a switch {} to determine which struct to construct. Other ideas?

[invidian]

That said, what alternative do you have in mind @invidian? The simplest approach is probably to have a slice of strings which represents all known platforms/components. This slice can then be used in a switch {} to determine which struct to construct. Other ideas?

I personally prefer map + lookup by key, as it's more compact than a switch {} statement. But yeah, we would need something like this exactly.

However, I don't think constructing structs on our own in central place is a good approach, because right now we have this newComponent() functions in components, which actually provides default configurations for the components, which I think is a good feature (that we have a default configs and that we keep them in the component package itself) and we should preserve that.

So I'd do something like map[string]func() Component or map[string]func() ComponentConfig and select from there.

[johananl]

As you know I'd rather be a bit more elaborate but clearer than compact and clever. A map of functions feels very "clever" to me and isn't something I'd expect to encounter as a developer who is new to the codebase. Anyway, we can discuss things in more detail once there is something implemented since then the pros/cons of the chosen approach would be clearer.

[invidian]

So, what I propose is the following:

package components

import (
  "fmt"

  "github.com/kinvolk/lokomotive/pkg/components/openebs"
  "github.com/kinvolk/lokomotive/pkg/components/dex"
)

func DefaultConfig(name string) (Component, error) {
  c := map[string]func() Component {
    openebs.Name: openebs.DefaultConfig,
    dex.Name:     dex.DefaultConfig,
    // We could also call config here, but then it will be called for all components.
    // dex.Name:     dex.DefaultConfig(),
  }

  if f, ok := c[name]; ok {
    return f()
  }

  return fmt.Errorf("component %q not found")
}

What I understand @johananl proposes is the following (if I'm mistaken, please edit this comment):

package components

import (
  "fmt"

  "github.com/kinvolk/lokomotive/pkg/components/openebs"
  "github.com/kinvolk/lokomotive/pkg/components/dex"
)

func DefaultConfig(name string) (Component, error) {
  switch name {
  case openebs.Name:
    return openebs.DefaultConfig()
  case dex.Name:
    return dex.DefaultConfig()
  default:
    return fmt.Errorf("component %q not found")
  }

  //panic("should never happen")
  return fmt.Errorf("should never happen")
}

I think what we agree on is the following:

• Each component package should export function providing default component configuration (so user configuration can be loaded on top of it).
• components.DefaultConfig() (or whatever we call this function) should return component's default configuration if component exists or error otherwise.

And arguments for using map over switch/case statement:

• Map allows more declarative approach, as data declaration is separate from the code (you have 2 parts of the functions, which could even be 2 separate functions, one to declare a map and other part to use it).
• Using map eliminates using N return statements and dead code path at the end OR an assignment statement (so you always return the variable at the end).
• Using map also eliminates boilerplate case and x = code for every supported component.

[invidian]

Created PR #1147 to definitely address this.