feat(metadata): raise error when capability/dependency not resolved in CamelCatalog

[tadayosi]

It's not needed to include it in 1.10.0 release. I've just submitted this in advance. Let's wait for 1.10.0 release and CI to become stable again.

It's important to see CI passes with this fix as it will change the way Operator handles integration builds.

Fix #3449

Release Note

feat(metadata): raise error when capability/dependency not resolved in CamelCatalog

[squakez]

LGTM. Just wondering if the unit test is enough or we may want to add also an E2E test to validate any errors on runtime as well.

[tadayosi]

@squakez @astefanutti
Along with adding E2E, I found that currently there's no phase for an integration when the initialisation action failed (like components / capabilities not resolvable in the catalog), which leads an integration to stay in empty phase. Thus I introduced a new phase Init Error for the initialisation failure for an integration.

I'd like to make sure I'm in the right direction so please review again. Thanks.

[squakez]

I think the last commits are fixing what we've discussed in #3389 (comment) - we should verify that once merged.

[astefanutti]

I'm wondering if using the existing Error phase, and introducing an Initialisation Failed condition could be an alternative? Same for the Platform Ready phase? I'd advocate for keeping the number of phases minimal as it helps reasoning about it. Generally, "touch points" like "platform ready" are captured by conditions rather than phases, that control the state machine.

[tadayosi]

I'm wondering if using the existing Error phase, and introducing an Initialisation Failed condition could be an alternative?

Good point. In this case, the monitor action would keep monitoring the failed integration, so it would need to be changed so that it keeps ignoring integrations with the Initialisation Failed condition in its Handle logic. Is it what you're suggesting?

I thought it doesn't make much sense to keep monitoring the integrations with Initialisation Failed as it wouldn't be able to recover anyway.

Same for the Platform Ready phase? I'd advocate for keeping the number of phases minimal as it helps reasoning about it. Generally, "touch points" like "platform ready" are captured by conditions rather than phases, that control the state machine.

Actually Platform Ready is a temporary pseudo phase and never persisted. The problem is that if the Platform trait changes to Initialisation phase right after its application it would also trigger subsequent traits in the platform setup action, not in the next initialisation action. It's because right now whether a trait is activated or not mostly depends on the integration phase. But maybe I can find a better logic to skip them in the Platform trait.

[astefanutti]

I'm wondering if using the existing Error phase, and introducing an Initialisation Failed condition could be an alternative?

Good point. In this case, the monitor action would keep monitoring the failed integration, so it would need to be changed so that it keeps ignoring integrations with the Initialisation Failed condition in its Handle logic. Is it what you're suggesting?

Exactly. I'm assuming the monitor action logic is already idem-potent to error cases, but it may need to be adjusted to accommodate that case as you say.

Same for the Platform Ready phase? I'd advocate for keeping the number of phases minimal as it helps reasoning about it. Generally, "touch points" like "platform ready" are captured by conditions rather than phases, that control the state machine.

Actually Platform Ready is a temporary pseudo phase and never persisted. The problem is that if the Platform trait changes to Initialisation phase right after its application it would also trigger subsequent traits in the platform setup action, not in the next initialisation action. It's because right now whether a trait is activated or not mostly depends on the integration phase. But maybe I can find a better logic to skip them in the Platform trait.

That "Actually Platform Ready is a temporary pseudo phase and never persisted" rather sounds like a better solution must exists :) I'd avoid piggy-backing on the state machine phases.

If I understand the problem correctly, maybe a solution would be to call the traits in two phases, rather than in a single sequence:

• a first phase calling the Configure method on the traits
• a second phase calling the Apply method on the traits enabled during the first phase

[tadayosi]

If I understand the problem correctly, maybe a solution would be to call the traits in two phases, rather than in a single sequence:

• a first phase calling the Configure method on the traits
• a second phase calling the Apply method on the traits enabled during the first phase

At first I thought it's a better solution but then found out that in a phase like deploying, in the Configure method phase some traits expect the outcomes of the Apply method of a previous trait, e.g. Service -> Container. This seems also a sensible design choice as otherwise the application chain of traits for a phase won't finish in a single batch and we might need to iterate several times for a phase to reach the desired state before moving to another phase, or we would simply end up omitting some important configs that should've been applied.

It might be also possible that we leak some trait configuration code into the Apply method in order to catch up the outcome of previous traits, but I don't think it's a good design.

Fow now, the original problem is only related to Platform trait so I'll try to fix it within the trait instead of overhauling the entire process.

What is most problematic with the original problem is that a trait changes the phase of integration while the phase is the only source for traits to decide whether they should be applied or not. The golden rule should be that traits must not change the phase of integration in a way that changes the set of applicable traits.

[tadayosi]

@astefanutti Revised to not introduce a new phase. Can you check this again please?

[tadayosi]

@astefanutti Applied the review comment.