canonical
Config: Don't panic and forward errors
This removes several panic() calls from the lxd/config package when trying to read configuration and instead forward the errors.
As the changes feel quite invasive now I am wondering if a better approach would be to instead perform the validation when setting the respective value. A read operation would then just return a valid value. This would also be in line with what we do for storage driver configuration (there we just read from map[string]string).
Any thoughts?
@roosterfish please can you explain further what has changed to require this now, or has the panic scenario always existed?
@roosterfish please can you explain further what has changed to require this now, or has the panic scenario always existed?
It has always existed, just saw it as part of the other story with the hidden passwords.
I am wondering if a better approach would be to instead perform the validation when setting the respective value. A read operation would then just return a valid value. This would also be in line with what we do for storage driver configuration (there we just read from
map[string]string).
Please could you give an example, i do not follow.
Please could you give an example, i do not follow.
Let's take the core.https_allowed_credentials server config setting as an example. It has the following getter function:
func (c *Config) HTTPSAllowedCredentials() bool {
return c.m.GetBool("core.https_allowed_credentials")
}
Removing the panic() which might happen as part of running GetBool(), requires to instead return an error from the getter:
func (c *Config) HTTPSAllowedCredentials() (bool, error) {
return c.m.GetBool("core.https_allowed_credentials")
}
For example right now if the value of core.https_allowed_credentials happens to not be a valid boolean, it panics. This should never be the case as it is already checked when setting the value.
I am wondering, if instead of causing a panic, we should rather forward the potential error message instead. This is what is currently proposed in the PR. But I see that this doesn't match to what we do for storage pool configuration.
The problematic piece is for the server config we have to move the setting stored as string back to their original data type (bool, int) which can cause an error by nature.
Do we even need the Config struct anymore?
Can we use a map everywhere like we do with the others and use shared.FalseOrEmpty etc
It would also allow resolving the different behaviors of defaults compared to rest of system
Can we use a map everywhere like we do with the others and use shared.FalseOrEmpty etc
That would be another option yes. For the bool case it's easy as FalseOrEmpty just returns a bool from the input string but there are many other occasions where we have to get an int from the string (which might cause an error too) which would also require additional handling.
Should this be closed or rebasee now?
I have it on the radar. But as the underlying structure is now again a map[string]any I have to validate the way forward. But will update here and close this PR if necessary.