equinor / configsuite

Config Suite is the result of recognizing the complexity of software configuration.
Other
7 stars 13 forks source link

Handle disallow required for containers #142

Closed lars-petter-hauge closed 4 years ago

lars-petter-hauge commented 4 years ago

The https://github.com/equinor/configsuite/commit/1768ccc5c03be44d74e4e3e1aaa29bbb8883c529 introduced disallowing required for containers. A typical usage of configuration is to have a list of named dicts (e.g. https://github.com/equinor/configsuite/blob/9c1eaeed3610ffaa9e549a35dc2709da44633c75/tests/data/car.py#L74 )

As it is no longer possible to set MK.Required: False, the end-point user would have to specify an empty dict, if there are no incidents, in order for the schema to be valid.

Or, as has been done in the test-data for the test/data/car.py, a default layer with empty container versions could be provided.

But that means that for any schema that contains a List of NamedDicts, where any of the elements in the NamedDicts is required, a package using configsuite must provide a default layer with an empty version of said container.

Is this the intended way?

lars-petter-hauge commented 4 years ago

I fear it will decouple how the schema will look like before end point user configs are added, to such an extent that it is difficult for the users of configsuite to read the schema. For more complex configuration this problem will likely be more extensive as well

lars-petter-hauge commented 4 years ago

Consider the following; We want the end point user to be able to specify a list of configurations, where each config consists of a named dict with some required and some not required variables:

"my_list": {
    MK.Required: True,
    MK.Type: types.List,
    MK.Content: {
        MK.Item: {
            MK.Type: types.NamedDict,
            MK.Content: {
                "key": {MK.Required: True, MK.Type: types.String},
                "key_property": {
                    MK.Required: False,
                    MK.Type: types.List,
                    MK.Content: {
                        MK.Item: {
                            MK.Type: types.Integer,
                        }
                    },
                },
            },
        }
    },
},

For the key entry, which is a basic type, we can add MK.AllowNone: False. However for the adittional key_property, it is not possible to state that it is no longer required. I don't see how this can be solved using default layers with empty containers as proposed above either, as that could not solve for the initial List.

markusdregi commented 4 years ago

Both List and Dict should default to the empty container and I consider the fact that it does not a bug. The same issue was reporter internally by @berland. I made some failing tests on Friday in https://github.com/equinor/configsuite/pull/141 and will address the problem ASAP. Do you agree that this will fix the issue?

lars-petter-hauge commented 4 years ago

Yes, that should fix the issue!

I believe we discussed the notion of returning an empty version of itself for containers, and there were arguments against it. But I can't remember what it was, and it might very well have been prior to including the AllowNone, which could have made said arguments mute.