Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change method of setting config defaults #192

Merged
merged 6 commits into from
Jun 21, 2024
Merged

change method of setting config defaults #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bf7ebf2
change method of setting config defaults
dustinblack Jun 19, 2024
7c90f7a
update engine test to use podman
dustinblack Jun 19, 2024
bf73a03
update load test to podman
dustinblack Jun 19, 2024
7046dd9
test go lint and test w/ podman added
dustinblack Jun 19, 2024
fb23b03
revert lint and test to action main branch
dustinblack Jun 19, 2024
ac6e415
simplify if condition
dustinblack Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 3 additions & 18 deletions cmd/arcaflow/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,18 +53,6 @@ func main() {
Stdout: os.Stderr,
})

defaultConfig := `
log:
level: info
deployers:
image:
deployer_name: podman
deployment:
imagePullPolicy: IfNotPresent
logged_outputs:
error:
level: info`

configFile := ""
input := ""
dir := "."
Expand Down Expand Up @@ -133,12 +121,9 @@ logged_outputs:
}

var configData any
if len(configFile) == 0 {
if err := yaml.Unmarshal([]byte(defaultConfig), &configData); err != nil {
tempLogger.Errorf("Failed to load default configuration", err)
os.Exit(ExitCodeInvalidData)
}
} else {
// If no config file is passed, we use an empty map to accept the schema defaults
configData = make(map[string]any)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if len(configFile) > 0 {
configFilePath, err := fileCtx.AbsPathByKey(RequiredFileKeyConfig)
if err != nil {
tempLogger.Errorf("Unable to find configuration file %s (%v)", configFile, err)
Expand Down
9 changes: 5 additions & 4 deletions config/load_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package config_test

import (
"go.arcalot.io/log/v2"
"testing"

"go.arcalot.io/log/v2"

"go.arcalot.io/lang"
"go.flow.arcalot.io/engine/config"
webbnh marked this conversation as resolved.
Show resolved Hide resolved
"gopkg.in/yaml.v3"
Expand All @@ -19,7 +20,7 @@ var configLoadData = map[string]struct {
expectedOutput: &config.Config{
TypeHintPlugins: nil,
LocalDeployers: map[string]any{
"image": map[string]string{"deployer_name": "docker"},
"image": map[string]string{"deployer_name": "podman"},
},
Log: log.Config{
Level: log.LevelInfo,
Expand All @@ -35,7 +36,7 @@ log:
expectedOutput: &config.Config{
TypeHintPlugins: nil,
LocalDeployers: map[string]any{
"image": map[string]string{"deployer_name": "docker"},
"image": map[string]string{"deployer_name": "podman"},
},
Log: log.Config{
Level: log.LevelDebug,
Expand Down Expand Up @@ -70,7 +71,7 @@ plugins:
"quay.io/arcalot/example-plugin:latest",
},
LocalDeployers: map[string]any{
"image": map[string]string{"deployer_name": "docker"},
"image": map[string]string{"deployer_name": "podman"},
},
Log: log.Config{
Level: log.LevelInfo,
Expand Down
2 changes: 1 addition & 1 deletion config/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func getConfigSchema() *schema.TypedScopeSchema[*Config] {
nil,
nil,
nil,
schema.PointerTo(`{"image": {"deployer_name": "docker"}}`),
schema.PointerTo(`{"image": {"deployer_name": "podman"}}`),
webbnh marked this conversation as resolved.
Show resolved Hide resolved
nil,
),
"logged_outputs": schema.NewPropertySchema(
Expand Down
5 changes: 3 additions & 2 deletions engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ package engine_test
import (
"context"
"errors"
"go.flow.arcalot.io/engine/loadfile"
"testing"

"go.flow.arcalot.io/engine/loadfile"

log "go.arcalot.io/log/v2"
"go.flow.arcalot.io/engine"
"go.flow.arcalot.io/engine/workflow"
Expand All @@ -29,7 +30,7 @@ func createTestEngine(t *testing.T) engine.WorkflowEngine {
cfg.Log.Level = log.LevelDebug
cfg.Log.Destination = log.DestinationTest
cfg.LocalDeployers["image"] = map[string]any{
"deployer_name": "docker",
"deployer_name": "podman",
"deployment": map[string]any{
"imagePullPolicy": "IfNotPresent",
},
Expand Down
Loading