You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/08/25 03:59:33 UTC
[GitHub] [camel-k] tadayosi opened a new pull request, #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
tadayosi opened a new pull request, #3571:
URL: https://github.com/apache/camel-k/pull/3571
<!-- Description -->
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.
Fix #3449
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
squakez commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1243351554
I think the last commits are fixing what we've discussed in https://github.com/apache/camel-k/issues/3389#issuecomment-1209143806 - we should verify that once merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on a diff in pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
tadayosi commented on code in PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#discussion_r969331891
##########
pkg/controller/integration/monitor.go:
##########
@@ -57,11 +57,28 @@ func (action *monitorAction) Name() string {
}
func (action *monitorAction) CanHandle(integration *v1.Integration) bool {
+ // When in InitializationFailed condition a kit is not available for the integration
+ // so the monitor action is not able to handle it.
+ if isInInitializationFailed(integration.Status) {
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
astefanutti commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1243368867
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1243396673
> 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?
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi merged pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
tadayosi merged PR #3571:
URL: https://github.com/apache/camel-k/pull/3571
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1244995571
@astefanutti Revised to not introduce a new phase. Can you check this again please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1245103401
@astefanutti Applied the review comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1243688581
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on a diff in pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
astefanutti commented on code in PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#discussion_r969243892
##########
pkg/controller/integration/monitor.go:
##########
@@ -57,11 +57,28 @@ func (action *monitorAction) Name() string {
}
func (action *monitorAction) CanHandle(integration *v1.Integration) bool {
+ // When in InitializationFailed condition a kit is not available for the integration
+ // so the monitor action is not able to handle it.
+ if isInInitializationFailed(integration.Status) {
Review Comment:
How is the integration going to move from error to running phase if it gets updated to fix the issue? My understanding is that the `monitor` action should still be applied to check whether the digest has changed? If that understanding is correct, the `isInInitializationFailed` check should be moved in the `Handle` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1243216567
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] astefanutti commented on pull request #3571: feat(metadata): raise error when capability/dependency not resolved in CamelCatalog
Posted by GitBox <gi...@apache.org>.
astefanutti commented on PR #3571:
URL: https://github.com/apache/camel-k/pull/3571#issuecomment-1243412940
> > 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org