Set the ValuesSchema and DefaultValues in Carvel pkgs
#4023
Conversation
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
There was a problem hiding this comment.
LGTM, though is it not possible for the rendered default values to be commented out as we'd discussed before the break? The Carvel team was pretty keen to be able to differentiate between values that are explicitly set by the user and default values, whereas generating them like this removes that distinction. I guess there are issues with rendering the default values commented out so the user can just uncomment any parts they want to set?
(I realise this would be different to helm, where we include the provided values, but it is a different situation - Carvel doesn't provide values.yaml, and the carvel team seem pretty particular about this, so might be worth investigating).
| // In short, it differs from upstream in that: | ||
| // -- 1. Prevent deep copy of int as it panics | ||
| // -- 2. For type object scan the first level properties for any defaults to create an empty map to populate | ||
| // -- 3. If the property does not have a default, add one based on the type ("", false,) |
There was a problem hiding this comment.
Thanks for the details there.
| missingDefaultObject: {} | ||
| missingDefaultString: "" | ||
| valueWithDefault: 80 | ||
| `}, |
There was a problem hiding this comment.
Looks like this should be 7 test cases (you've even added the infra for iterating the test cases) that you've put in one test-case? (ie. it will fail if any of the parts fail). Not a big deal and fine as is, just unexpected.
There was a problem hiding this comment.
haha, you're right; I started writing the different cases at the same time, but I forgot to split them apart. Done (+ few additional test cases more)
| } | ||
|
|
||
| defaultValues(in, tt.schema) | ||
| if !reflect.DeepEqual(in, expected) { |
There was a problem hiding this comment.
I'm guessing there's a reason that the upstream change doesn't use cmp.Equal instead of reflect.DeepEqual? Either way we probably want to keep the code as similar as possible.
There was a problem hiding this comment.
Same... I don't know, but I tried to keep it as aligned as possible to the upstream version :S
I did try to comment them out and even add some comments with the An alternative would be re-implementing the default function to directly write the yaml nodes instead of relying on an ulterior marshaling process. However, it seemed non-trivial and given that we may want to add the "advanced form tab", it wouldn't be that useful then. What I have done, instead, is to comment out the whole generated string afterward (appending the
|
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
|
Looks awesome. +1 from me for providing them commented out. |
Description of the change
This PR is aimed at providing a set of initial values for users when installing/upgrading a Carvel package. Particularly, in this PR:
.Defaultproperty exists, use it; otherwise, fall back to each data type default value (eg.,""or0)server_utils_testwasn't being tested.Benefits
Users will get more information about the values that a package requires, being a similar UX as in other pkg formats (Helm).
Possible drawbacks
Some problems arise:
Fluent Bitpackage fails. In these cases, I've added a commented-out warning message.additionalProperties: string(where it should be either an object or a boolean; reported to the TCE pkgs team) Some pkgs from the TCE repo has an invalidadditionalProperties: stringcommunity-edition#2772Applicable issues
Additional information
IRL example:
