KEP-29: Add KudoOperatorTask implementation

[zen-dog]

Summary:
implemented KudoTaskOperator which, given a KudoOperator task in the operator will create the Instance object and wait for it to become healthy. Additionally added paramsFile to the KudoOperatorTaskSpec.

Fixes: #1509

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

[zen-dog]

Moved from the instance_contorller.go

[zen-dog]

Split this part out of the getParamsFromFiles method because I needed it for paramsFile expansion.

[zen-dog]

Failing operator-tests are due to quay.io outage

[kensipe]

this seems off...

1. errs as []string seems odd... initially thought it should be []error
2. I see the logic but feel we should improve it... we are concat all the error strings into 1 long error message.
3. the issue is there 2 types of "errors that are possible. 1) unmarshal error of size 1 array or 2) casting error of size n array.
4. it also feels odd to "return" errors based on passed array ref

It doesn't seem like a good idea to have an uncontrolled size of error message. I get wanting to "know" all the entries that are wrong... but then the error message from GetParametersFromFile should be must more brief in its error message.

I think I would rather have GetParametersFromFile append conversion errors into a string and have it be responsible for return the error with the concat err msg. Then we can have 1 error return and return it like a Go developer :). I'll add details need the code I'm referring to.

[kensipe]

not mentioned above... but it is also strange to pass in a ref to errs []string and also return that []string

[kensipe]

prefer the following:

func GetParametersFromFile(filePath string, bytes []byte, parameters map[string]string) error {
	data := make(map[string]interface{})
	err := yaml.Unmarshal(bytes, &data)
	if err != nil {
		return fmt.Errorf("error unmarshalling content of parameter file %s: %w", filePath, err)
	}
	clog.V(2).Printf("Unmarshalling %q...", filePath)
	castErrors := []string{}
	for key, value := range data {
		clog.V(3).Printf("Value of parameter %q is a %T: %v", key, value, value)
		var valueType v1beta1.ParameterType
		switch value.(type) {
		case map[string]interface{}:
			valueType = v1beta1.MapValueType
		case []interface{}:
			valueType = v1beta1.ArrayValueType
		case string:
			valueType = v1beta1.StringValueType
		default:
			valueType = v1beta1.StringValueType
		}
		wrapped, err := convert.WrapParamValue(value, valueType)
		if err != nil {
			castErrors = append(castErrors, fmt.Sprintf("%s", key))
			//errs = append(errs, fmt.Sprintf("error converting value of parameter %s from file %s %q to a string: %v", key, filePath, value, err))
			continue
		}
		parameters[key] = *wrapped
	}
	if len(castErrors) > 0 {
		return fmt.Errorf("error converting values to strings from file %s for the following keys [%s]", strings.Join(castErrors, ", "))
	}
	return nil
}

[kensipe]

with details of err in a clog.V(x) log

[zen-dog]

These are all valid points, but I merely extracted this part from the getParamsFromFiles method above and kept the overall structure. This is the way this parser is built starting with the entry method GetParameterMap above: all the helper methods are treating errors as strings gathering them together.

Imho the biggest drawback of the above approach is that the individual errors collected in this line:

errs = append(errs, fmt.Sprintf("error converting value of parameter %s from file %s %q to a string: %v", key, filePath, value, err))

are lost and replaced by a generic error converting values to strings without extra details of what exactly is the problem.

[zen-dog]

Ok, I reworked it and the end result is pretty close to the variant above: verbosity is reduced and the method returns an error. However, I keep the original error with the corresponding key.

[ANeumann82]

I'm pretty sure this is not enough to correctly patch the instance, I want to see at least a test that removes a parameter from the updated instance. This could for example happen if the paramsFile for the dependent operator has something like:

{{ if eq ".Params.INCLUDE_STUFF }}
FANCY_PARAM_THAT_CAN_BE_THERE_OR NOT: "cool value"
{{ end }}

[ANeumann82]

Can we utilize the applyResources from the task_apply.go?

[zen-dog]

You're right. I've adopted this logic from kudo.go::UpdateInstance where we use the MergePatchType. But switching to the StrategicMergePatchType will require storing the previous state but instance controller is already doing that... I've created an issue #1544

[zen-dog]

The applyResources sets all the labels and annotations but more importantly, it sets the kudo.dev/last-applied-configuration. However, an Instance already has it set by the instance_controller.go called kudo.dev/last-applied-instance-state, which is something I wanted to removed altogether. What do you think about setting patchStrategy:"merge" for the spec.parameters like:

type InstanceSpec struct {
	...
	Parameters map[string]string `json:"parameters,omitempty" patchStrategy:"merge"`
}

At least in the kudo_operator_task.go we generate a full map of parameters each time so overriding everything should work, right?

[nfnt]

Thanks, this is looking great now! Got one nitpick and it would be great to rename KudoOperatorTaskSpec.Packages as well.