feat: allow return additional information for conditions

[metacosm]

Fixes #2424. Currently, only exposed for ready post conditions but is
supported for all conditions if needed.

Signed-off-by: Chris Laprun [email protected]

[csviri]

One thing we should consider is to simply change the Condition since this is for v5. And have an optional result always.

[metacosm]

Makes sense. Will do!

[metacosm]

Hmm, on second thought, it might make sense to leave it as-is because moving this to Condition would trigger needed changes to all existing conditions even if they don't have any interest in returning a result (unless, of course, we don't want to make it generic and just return Object as the result).

[csviri]

Could it be just as a Matcher result? Simply factory methods

[csviri]

That would really not be a big issue to migrate.

[metacosm]

This isn't comparable to Matcher.Result: this isn't a result class, this is a class that can return a result so it is quite different.

[csviri]

I meant the Result interface not the ResultCondition.

[csviri]

I still think that we should not complicate things and have a separate ResultCondition and Condition, since we work on v5, putting this directly to condition is completely fine IMO, and can be easily migrated in a controller simply one line code change.

[metacosm]

I'd do that in a second step if you feel it's important. I'll let you check the impact on existing code first (hint: it's not a one line change 😉).

[csviri]

I think we should add this to all conditions, and the functionality directly to Condition. What do you think, @metacosm ?

[metacosm]

The problem with adding it to all conditions is that we will need to rethink how results are reported since the conditions are more or less invisible at this point in the results apart from the non-ready dependents.

[csviri]

The problem with adding it to all conditions is that we will need to rethink how results are reported since the conditions are more or less invisible at this point in the results apart from the non-ready dependents.

yes, what about putting here, into WorkflowReconcileResult:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java

Lines 12 to 14 in 4105421

	private final List<DependentResource> reconciledDependents;
	private final List<DependentResource> notReadyDependents;
	private final Map<DependentResource, ReconcileResult> reconcileResults;

instead of List<DependentResource> a new object representing the reconciled node with also all the condition results. Something like List<DependentResourceNodeResult>:

class DependentResourceNodeResult {

DependentResource getDependentResource();

Optional<ReconcileResult> getReconcileResult();

Optional<ConditionResult> getReadyConditionResult();
Optional<ConditionResult> getReconcilePreconditionResult();
Optional<ConditionResult> getCleanupPostconditionResult();
}

[metacosm]

The problem with adding it to all conditions is that we will need to rethink how results are reported since the conditions are more or less invisible at this point in the results apart from the non-ready dependents.

yes, what about putting here, into WorkflowReconcileResult:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileResult.java

Lines 12 to 14 in 4105421

	private final List<DependentResource> reconciledDependents;
	private final List<DependentResource> notReadyDependents;
	private final Map<DependentResource, ReconcileResult> reconcileResults;

instead of List<DependentResource> a new object representing the reconciled node with also all the condition results. Something like List<DependentResourceNodeResult>:

class DependentResourceNodeResult {

DependentResource getDependentResource();

Optional<ReconcileResult> getReconcileResult();

Optional<ConditionResult> getReadyConditionResult();
Optional<ConditionResult> getReconcilePreconditionResult();
Optional<ConditionResult> getCleanupPostconditionResult();
}

I have some ideas that I'm working on, hopefully will have something to show today but, yes, something of that order, though I wasn't sure I'd expose the class equivalent to DependentResourceNodeResult.

[csviri]

Added some comments and quiestion.

Could you pls update or add an integration test that will serves as a sample / show it working in practice?

[csviri]

Could it be just as a Matcher result? Simply factory methods

[csviri]

That would really not be a big issue to migrate.

[csviri]

Can't we have this as a builder rather? this feels a bit smelly to have it as an interface

[metacosm]

Not sure I'm following how a builder would help here.

[csviri]

Can we see this working in an integration test, that might shed a light on it.

[metacosm]

See fd0e92c#diff-a2db869773190cffe048fc2b938d7808bf57160f274af63fde8e2b8cfc0443f8R188-R196

[csviri]

Please update docs with the new type of condition and how to access results, with links to the integration tests

[metacosm]

Please update docs with the new type of condition and how to access results, with links to the integration tests

Added documentation.