wip: allow passing defs path

[supakeen]

This is mostly a request for comments as everything here is work-in-progress. I'd really like it if we can pass the definitions path directly from image-builder without having to go through an environment variable but I don't quite like what I've done here.

Does someone know a better way to do this?

An open problem is ParseID which directly loads definitions without going through the loader's state.

[achilleas-k]

The thing I don't like about this is that a lot of functions now have an (only or first) argument that in many cases will be set to an empty string to get the default behaviour. I think that if the defs dir being set externally is the exception, then this should be reflected in the way the functions are called and the types are initialised. It should be simpler to get the common default behaviour than the exceptional one. So, instead of:

d := distrofactory.NewDefault("")  // default case

e := distrofactory.NewDefault(somevalue) // exceptional case

I find it's nicer to have

d := distrofactory.NewDefault()  // default case

e := distrofactory.NewDefault()
if defsPath != "" {
    e.DefsPath(defsPath)  // exceptional case
}

[supakeen]

The thing I don't like about this is that a lot of functions now have an (only or first) argument that in many cases will be set to an empty string to get the default behaviour. I think that if the defs dir being set externally is the exception, then this should be reflected in the way the functions are called and the types are initialised. It should be simpler to get the common default behaviour than the exceptional one. So, instead of:

d := distrofactory.NewDefault("")  // default case

e := distrofactory.NewDefault(somevalue) // exceptional case

I find it's nicer to have

d := distrofactory.NewDefault()  // default case

e := distrofactory.NewDefault()
if defsPath != "" {
    e.DefsPath(defsPath)  // exceptional case
}

This is nicer, I'll rewrite it to have a DefinitionsLoader with a NewDefaultDefinitionsLoader and a .SearchPath (or such).

[thozza]

The thing I don't like about this is that a lot of functions now have an (only or first) argument that in many cases will be set to an empty string to get the default behaviour. I think that if the defs dir being set externally is the exception, then this should be reflected in the way the functions are called and the types are initialised. It should be simpler to get the common default behaviour than the exceptional one. So, instead of:

d := distrofactory.NewDefault("")  // default case

e := distrofactory.NewDefault(somevalue) // exceptional case

I find it's nicer to have

d := distrofactory.NewDefault()  // default case

e := distrofactory.NewDefault()
if defsPath != "" {
    e.DefsPath(defsPath)  // exceptional case
}

I agree, but would like to offer two different approaches that may be nicer and more idiomatic in the long run.
Especially when setting the DefsPath needs to basically throw away all definitions loaded by the NewDefault()

Functional options pattern

package distrofactory

type option func(*Factory)

func WithDefsPath(path string) option {
    return func(c *Factory) {
        // Do the loading from the custom path here.
    }
}

func NewDefault(opts ...option) *Factory {
    f := getDefault()

    // In our case, the logic would be different, because we would probably construst a new Factory object from scratch from the different path.
    for _, opt := range opts {
        opt(f)
    }
    return f
}

f := distrofactory.NewDefault()  // default case

f := distrofactory.NewDefault(distrofactory.WithDefsPath("/dev/null"))  // exceptional case

IMHO, this approach is the most efficient, given how distro defs are loaded and how they work. This is also a standard when it comes to big libraries like cloud provider SDKs.

Dysfunctional options pattern

(inspired by https://rednafi.com/go/dysfunctional_options_pattern/#dysfunctional-options-pattern)

package distrofactory

func NewDefault() *Factory {
    f := getDefault()
    return f
}

func (f *Factory) WithDefsPath(path string) *Factory {
    f.doSomething(path)
    return f
}

f := distrofactory.NewDefault()  // default case

f := distrofactory.NewDefault().WithDefsPath("/dev/null")  // exceptional case

Not ideal, because defs would be loaded twice AFAICT.

[thozza]

An open problem is ParseID which directly loads definitions without going through the loader's state.

I didn't inspect the code flow too deeply, but could we maybe not use distroid.NewDefaultParser(), but instead use the New(parsers ...ParserFunc) and pass the parser funcs from the loaded distro definitions? Or even make the distro ID parser construction a method of distro defs structure?

[achilleas-k]

Dysfunctional options pattern

package distrofactory

func NewDefault() *Factory {
    f := getDefault()
    return f
}

func (f *Factory) WithDefsPath(path string) *Factory {
    f.doSomething(path)
    return f
}

f := distrofactory.NewDefault()  // default case

f := distrofactory.NewDefault().WithDefsPath("/dev/null")  // exceptional case

This is nice. I like it! Maybe we should start doing this everywhere :)

Not ideal, because defs would be loaded twice AFAICT.

I was about to say we can delay loading until it's necessary but I think loading only happens when getDistro() is called already

images/pkg/distrofactory/distrofactory.go

Lines 29 to 41 in dbc3543

	func (f *Factory) getDistro(name string) distro.Distro {
	var match distro.Distro
	for _, f := range f.factories {
	if d := f(name); d != nil {
	if match != nil {
	panic(fmt.Sprintf("distro ID was matched by multiple distro factories: %v, %v", match, d))
	}
	match = d
	}
	}
	return match
	}