Support legacy boolean yes|no|on|off but warn user about incompatibility
#382
Conversation
loader/loader_test.go
Outdated
| } | ||
|
|
||
| func TestLoadLegacyBoolean(t *testing.T) { | ||
| _, err := loadYAML(` |
There was a problem hiding this comment.
Since the goal is to support legacy booleans, actual checking should not be removed in the test code.
There was a problem hiding this comment.
Sorry we forgot to update the issue title, we won't support the legacy option anymore as they are deprecated since 2009
There was a problem hiding this comment.
I agree that you don't want to support legacy YAML in the future.
However, Docker Compose v2.16 (compose-go v1.12.0) did support legacy YAML. If you are going to make breaking changes, you should do it when we bump the major version using semantic versioning.
As @ndeloof initially proposed, it is a good idea to display warnings in the current major version and make updates that will stop support in the future.
This approach will help users prepare for the changes and avoid any sudden disruptions.
loader/interpolate.go
Outdated
| case "false": | ||
| return false, nil | ||
| case "y", "yes", "on": | ||
| return true, fmt.Errorf("%q for boolean is not supported by YAML 1.2, please use `true`", value) |
There was a problem hiding this comment.
If the toBoolean function returns err, case map[string]interface{}: of the recursiveInterpolate function in interpolation/interpolation.go will set the output to nil.
With the current structure of the code, it seems to be difficult to successfully load with warnings.
Wouldn't it be better to just fix the case string: in the recursiveInterpolate function in interpolation/interpolation.go, just restore the legacy boolean support, and put the warning in a separate PR?
glours
changed the title
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof
changed the title
yes|no|on|off but warn user about incompatibility
|
After debate within the team, we decided to prefer backward compatibility, warning user about this misuse of yaml, and will plan removal for this legacy syntax in a future release |
No description provided.