ssa: Improve wait error reporting

[stefanprodan]

If the context deadline has not been reached, include only the failed resources in the returned error. This makes the health check error message concise, instead of listing potentially hundreds of resources in progress, we only list the ones that actually failed.

Part of: fluxcd/flux2#4556
Fixes: #715
Supersedes: #717

[souleb]

wdyt think about

 var errs []error
	for id, rs := range statusCollector.ResourceStatuses {
		switch {
		case rs == nil || lastStatus[id] == nil:
			errs = append(errs, fmt.Errorf("resource %s not found", utils.FmtObjMetadata(id)))
		case lastStatus[id].Status == status.FailedStatus:
			var builder strings.Builder
			builder.WriteString(fmt.Sprintf("%s status: '%s'",
				utils.FmtObjMetadata(rs.Identifier), lastStatus[id].Status))
			if rs.Error != nil {
				builder.WriteString(fmt.Sprintf(": %s", rs.Error))
			}
			errs = append(errs, errors.New(builder.String()))
...
	if len(errs) > 0 {
		msg := "failed early due to stalled resources"
		if errors.Is(ctx.Err(), context.DeadlineExceeded) {
			msg = "timeout waiting for"
		}
		return fmt.Errorf("%s: %w", msg, errors.Join(errs...))
	}

Wrapping the errors can make it easier to inspect for calling funtions.

[stefanprodan]

This is more about humans than code, keep in mind that the resulting error ends up in Slack, PagerDuty, etc. Is error wrapping with 100+ errors easier to read and parse than an array?

[souleb]

e.Error() will return a string still. The difference is that for Join. It says The error formats as the concatenation of the strings obtained by calling the Error method of each element of errs, with a newline between each string.

[stefanprodan]

Ok but this will change the format and if people rely on the current array, their parser will fail. I would consider change it in Flux minor release instead of a patch like now.

[darkowlzz]

LGTM!
Left a suggestion about simplification of duplicate code in the switch cases. Can be ignored or accepted.

[darkowlzz]

Feel free to ignore it but the two cases above seem identical.
Maybe there are two cases to keep the conditionals simple but maybe it'd be simpler to maintain in the long run if the duplicate code can be put together in one case and the case expression can be made a little complex like:

case (lastStatus[id].Status == status.FailedStatus) || (errors.Is(ctx.Err(), context.DeadlineExceeded) && lastStatus[id].Status != status.CurrentStatus):
        ...

I may be missing something if they aren't identical. Please tell me if they aren't.

[stefanprodan]

I did like this so it's easier to read, failure vs timeout.