change method of setting config defaults

[dustinblack]

Changes introduced with this PR

A change in PR #182 used an overly-complicated method of setting the defaults for the configuration. The existing schema definition already had defaults assigned, and in fact that change introduced a bug whereby a partial configuration file passed without a deployer defined will in fact default to docker rather than podman.

This update changes the default in the schema to podman, and it removes the statically-defined default config, opting instead to pass an empty map to the config.Load function, which has the effect of using the schema-defined defaults.

---

By contributing to this repository, I agree to the contribution guidelines.

[dustinblack]

Needs arcalot/arcaflow-reusable-workflows#23 in order for the github actions to pass
Apparently the action failures I saw after this initial commit were transient and not related to podman being missing from the reusable workflow, since for later commits the action has completed successfully from the main branch of the reusable workflow. ¯\(ツ)/¯

[dbutenhof]

Nice discovery, and I thought the long raw string was a bit ugly so I'm happy to lose it. Dropping the IfNotPresent should be fine because (if I'm reading it correctly) that's the default in the podman deployer schema. And info is the default log level as well, so, aside from possible minor issues of code style (😁) I think this looks good.

[dbutenhof]

Nicer. I'm not entirely sure in "go internals" how much difference there is between "missing" and "empty" maps, but I think the call to make should be unnecessary here.

[dbutenhof]

I think the canonical way to set a map to nil is to just declare the variable with a type; the default value is nil rather than empty, which may be more efficient. (Although the details of Go behind-the-scenes memory management is obscure 😆 )

That is, var configData map[string]any ... although are there any config map values that aren't strings? Isn't it really map[string]string?

[webbnh]

I concur with Dave: if you declare configData with the proper type (map[string]any), then Go will implicitly initialize it with the appropriate "nil" value for that type (which, in this case, would be map[string]any{}).

Note that this "nil" value is not the same as nil (for a map), and I believe that it is not the same as what is returned by make(map[string]any) (which is a map with no keys -- an empty map -- not a nil map). However, the nil map, as Dave indicated elsewhere, should behave the same as an empty map for read accesses, and it should be more efficient to instantiate and handle.

In any case, it would be good to combine lines 123 and line 125 and declare and initialize the variable in a single statement. It's just a happy coincidence that you can probably omit the explicit value from that initialization. 🙂

[dustinblack]

I can't seem to find the right way to do this. Everything I try results in the IDE complaining somewhere. It makes sense to me to simply declare the var up front as var configData map[string]any, but that breaks the loadYamlFile() function call at line 134.

[dbutenhof]

It's a CLI, so "efficiency" is of somewhat limited use. But I'm really confused if the loadYamlFile is complaining, since it's overwriting the current value of configData, and shouldn't care. Isn't it??? 😕 What's the IDE complaint?

[dustinblack]

cannot use loadYamlFile(configFilePath) (value of type any) as map[string]any value in assignment: need type assertioncompiler[IncompatibleAssign](https://pkg.go.dev/golang.org/x/tools/internal/typesinternal#IncompatibleAssign)

[webbnh]

/me sighs.

loadYamlFile() is a wrapper around yaml.Unmarshal() which takes an output parameter of out interface{} (aka any), because, more or less, it can return anything (or, at least, any value that you could encode in YAML).

So, somewhere along the line, we need to convert the output value from type any to the type that we expect to get based on what we know about the input that we supplied.

I suppose that that point is in this code after the call to loadYamlFile().

So, we need something like this:

		configDataRaw, err := loadYamlFile(configFilePath)
		if err != nil {
			tempLogger.Errorf("Failed to load configuration file %s (%v)", configFile, err)
			flag.Usage()
			os.Exit(ExitCodeInvalidData)
		}
		configData, ok = configDataRaw.(map[string]any)
		if !ok {
			tempLogger.Errorf("Invalid configuration file %s, does not contain a mapping", configFile)
			flag.Usage()
			os.Exit(ExitCodeInvalidData)
		}

This will "convert" the value in configDataRaw from an any type to the mapping type; if the conversion fails, it reports the error and exits.

[dbutenhof]

Ugh. So GitHub only finds this one caller to the unexported loadYamlFile. Are there more? Is there any conceivable reason it should allow returning anything other than a map?

The actual yaml.Unmarshall reacts to the type of the output pointer, so is the only problem here that loadYamlFile blindly tries to force an any return type?

[webbnh]

This is the only caller I found, but, given the name of loadYamlFile, it seems like it should have a more generic interface than forcing it to return a map (since "a file" could have lists or strings or things other than mappings in it).

yaml.Unmarshall() accepts an any parameter, so it is up to its caller to decide what the type should be (but Unmarshall() does a compatibility check against the data). The problem is that loadYamlFile() is a blind intermediary between the "real caller" and Unmarshall() so it cannot return anything but an any (unless we make it "opinionated", which I'm not really inclined to do) because it has no idea what the caller expects.

So, we're left with having the caller apply a type assertion...which, perhaps, is not that big of a deal.

[dustinblack]

@webbnh Your code suggestion does not work as-is (ok is undefined). At this point, I think the existing solution is how we should move forward. I feel like we are trying to untangle something that amounts to an annoyance with significantly more code and effort.

[webbnh]

The code as I presented it above would result in a compiler error (undefined: ok) if you took it literally as-is, because the variable ok is never declared.

But, it probably would have worked if you had added var ok bool or if you had used := to assign the result of the type assertion (I didn't supply those, because it wasn't clear which was better or where to put the declaration)...but, see below.

I feel like we are trying to untangle something that amounts to an annoyance

What we're trying to do is to get back to the safety of using strongly-typed variables. But, now that I look at it, that appears to be awkward at best: this code takes the value returned by loadYamlFile() (which is an any type, hopefully one containing a map value) and passes it to config.Load() which just forwards it to getConfigSchema().UnserializeType() which is a generic function that coerces its return value to the type that its caller wants (which appears to be *Config).

So, while the type should be a map[string]any (e.g., because that is what getConfigSchema().UnserializeType() needs in order to produce a *Config), the value in configData needs to be compatible with what loadYamlFile() returns, which is an any...so, we're kind of trapped by circumstance. That is, loadYamlFile() is conspiring with config.Load() to produce a reasonable result, and we unfortunately landed in between them.

with significantly more code and effort.

Actually, it was one type assertion and an error handler for it. (Hopefully, it will turn out that that error handler is redundant with the one for config.Load().)

But, if you're interested in reducing the code, I think that the separate initialization of configData at line 125 is superfluous: you could have done that on the declaration at line 123, and, as we pointed out, you could use the "zero value" for the map type.

[webbnh]

I second Dave's comments: this is a very nice improvement, but I don't think we need to explicitly create an empty map for the configuration. Aside from that, I tripped over a tiny nit.

the action failures I saw after this initial commit were transient

I haven't looked into this, but I vaguely recall other instances of the CI failing specifically on the first run, and that it works fine on subsequent runs. Very disappointing, but c'est la vie.

[webbnh]

I concur with Dave: if you declare configData with the proper type (map[string]any), then Go will implicitly initialize it with the appropriate "nil" value for that type (which, in this case, would be map[string]any{}).

Note that this "nil" value is not the same as nil (for a map), and I believe that it is not the same as what is returned by make(map[string]any) (which is a map with no keys -- an empty map -- not a nil map). However, the nil map, as Dave indicated elsewhere, should behave the same as an empty map for read accesses, and it should be more efficient to instantiate and handle.

In any case, it would be good to combine lines 123 and line 125 and declare and initialize the variable in a single statement. It's just a happy coincidence that you can probably omit the explicit value from that initialization. 🙂

[dbutenhof]

It's a CLI, so "efficiency" is of somewhat limited use. But I'm really confused if the loadYamlFile is complaining, since it's overwriting the current value of configData, and shouldn't care. Isn't it??? 😕 What's the IDE complaint?