Background
We should prioritize these two action items for our manifest's environment overrides:
- We should improve our unit test to make sure that all our existing and new fields can be overridden properly.
- Then, we should upgrade
mergo to the latest version (v0.3.12), and fix our code accordingly.
Unit tests
What's the issue
Our current unit test doesn't capture enough scenarios of environment override.
For example, in both #2442 and #2490, a non-empty list is overridden by an empty list, causing the default configuration being nullified by empty environment configuration. We should write tests so that these kinds of scenarios is captured.
What to do
We can figure out a way to loop through each manifest field, possibly with the help of reflect package, and ensure that the overrides work for all of the fields.
Alternatively, we can write unit tests for each one of the fields, and establish a protocol that for each new field introduced in manifest, we should write a separate and comprehensive test suit for that field.
mergo
Why have we been using an older version?
Because one of the newer versions introduced a behavior that would otherwise break our manifest, as discussed here.
Why do we need the newer versions?
The newer versions have fixed a bug that affects our code.
Previously, the mergo.WithOverwriteWithEmptyValue option didn't overwrite a pointer with a nil pointer, which is a bug since a nil pointer is an empty value. This is fixed in the newer releases.
This has impact on our code, for example, on CommandOverride and Network (#2442 and #2490). There may be some
similar impact that we should investigate.
What to do
After improving the unit tests, we should upgrade mergo, and implement custom logics such that we can preserve behaviors discussed here. We should also be mindful to investigate what's changed in the latest release, and fix our code accordingly so that we don't introduce any breaking changes to our user.
Background
We should prioritize these two action items for our manifest's environment overrides:
mergoto the latest version (v0.3.12), and fix our code accordingly.Unit tests
What's the issue
Our current unit test doesn't capture enough scenarios of environment override.
For example, in both #2442 and #2490, a non-empty list is overridden by an empty list, causing the default configuration being nullified by empty environment configuration. We should write tests so that these kinds of scenarios is captured.
What to do
We can figure out a way to loop through each manifest field, possibly with the help of
reflectpackage, and ensure that the overrides work for all of the fields.Alternatively, we can write unit tests for each one of the fields, and establish a protocol that for each new field introduced in manifest, we should write a separate and comprehensive test suit for that field.
mergoWhy have we been using an older version?
Because one of the newer versions introduced a behavior that would otherwise break our manifest, as discussed here.
Why do we need the newer versions?
The newer versions have fixed a bug that affects our code.
Previously, the
mergo.WithOverwriteWithEmptyValueoption didn't overwrite a pointer with anilpointer, which is a bug since anilpointer is an empty value. This is fixed in the newer releases.This has impact on our code, for example, on
CommandOverrideandNetwork(#2442 and #2490). There may be somesimilar impact that we should investigate.
What to do
After improving the unit tests, we should upgrade
mergo, and implement custom logics such that we can preserve behaviors discussed here. We should also be mindful to investigate what's changed in the latest release, and fix our code accordingly so that we don't introduce any breaking changes to our user.