You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "squakez (via GitHub)" <gi...@apache.org> on 2024/03/08 08:19:42 UTC
[PR] fix(trait): revert persisted status [camel-k]
squakez opened a new pull request, #5233:
URL: https://github.com/apache/camel-k/pull/5233
The work done in #5153 is based on the wrong assumption that a trait status can be persisted at any point of the Integration/IntegrationKit lifecycle This cannot be always true, because the resource move towards different .status.phase with the execution of its logic or not depending on the phase. Therefore we cannot persist in a consistent way, as the status would reflect the latest execution of a trait which may happen in a different phase (and would be overridden by the new execution phase). The best way to deal with this, at this stage, seems to be the usage of .status.conditions in order to warn that some trait value was overwritten.
<!-- Description -->
<!--
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
fix(trait): revert persisted status
```
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985317921
>
> so I'm a little bit confusing about what is the `.status.conditions` we are talking about here :)
NP. Let me try to summarize in shorter terms. `.status.conditions` already exists and it is meant to provide some info about possible info/warnings the user needs to know. Sometime ago I introduced another field, `.status.traits` whose goal was to store the executed traits in order to make the operator logic more consistent. However, this development was made based on the wrong assumptions provided in this PR description. The aim of this PR is to remove `.status.traits` which is inconsistent. Any user interested in troubleshooting an Integration can still rely on `.status.conditions` as it was doing previously.
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985286813
> > The best way to deal with this, at this stage, seems to be the usage of .status.conditions in order to warn that some trait value was overwritten.
>
> In general, I would recommend not to rely on conditions as those are primarily designed to be consumed by observers rather than the operator itself. A golden rule of controller is that, with some very little exceptions, the `status` sub resource should not be used to determine the current state.
Yes. The point for the consume of conditions was more directed to a human observer that wants to understand why, for instance, an Quarkus native Integration is instructing the Kit to use 4GB of memory (which is the sensible default configuration the operator overrides).
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985815518
```
--- PASS: TestNativeHighMemoryIntegrations (1834.10s)
--- PASS: TestNativeHighMemoryIntegrations/java_native_support (875.74s)
--- PASS: TestNativeHighMemoryIntegrations/java_native_support/java_native_same_should_not_rebuild (2.21s)
--- PASS: TestNativeHighMemoryIntegrations/java_native_support/java_native_should_rebuild (428.39s)
--- PASS: TestNativeHighMemoryIntegrations/groovy_native_support (495.43s)
--- PASS: TestNativeHighMemoryIntegrations/kotlin_native_support (451.77s)
PASS
ok github.com/apache/camel-k/v2/e2e/native 1834.133s
--- PASS: TestNativeIntegrations (884.52s)
--- PASS: TestNativeIntegrations/unsupported_integration_source_language (0.76s)
--- PASS: TestNativeIntegrations/xml_native_support (412.21s)
--- PASS: TestNativeIntegrations/automatic_rollout_deployment_from_jvm_to_native_kit (462.53s)
--- PASS: TestNativeIntegrations/automatic_rollout_deployment_from_jvm_to_native_kit/yaml_native_should_not_rebuild (2.70s)
PASS
ok github.com/apache/camel-k/v2/e2e/native 1330.983s
```
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985320919
> > so I'm a little bit confusing about what is the `.status.conditions` we are talking about here :)
>
> NP. Let me try to summarize in shorter terms. `.status.conditions` already exists and it is meant to provide some info about possible info/warnings the user needs to know. Sometime ago I introduced another field, `.status.traits` whose goal was to store the executed traits in order to make the operator logic more consistent. However, this development was made based on the wrong assumptions provided in this PR description. The aim of this PR is to remove `.status.traits` which is inconsistent. Any user interested in troubleshooting an Integration can still rely on `.status.conditions` as it was doing previously.
ok then, I was a little bit confused because the `.status.traits` has been the center of a number of things recently, like checksum, copy to kit, etc so I missed a little bit the context.
sorry for the noise
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985281899
> The best way to deal with this, at this stage, seems to be the usage of .status.conditions in order to warn that some trait value was overwritten.
>
In general, I would recommend not to rely on conditions as those are primarily designed to be consumed by observers rather than the operator itself. A golden rule of controller is that, with some very little exceptions, the `status` sub resource should not be used to determine the current state.
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985324027
> ok then, I was a little bit confused because the `.status.traits` has been the center of a number of things recently, like checksum, copy to kit, etc so I missed a little bit the context.
>
> sorry for the noise
Probably I was the one making noise with wrong development ;) - Fixing all those wrong stuff though.
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985310240
> > should that be part of the kit condition/status then ?
>
> I think it is already happening. We stores conditions of certain traits overridden properties and deprecation notices. It may be polished a little bit (see #5027) but the logic to store them is already there.
so I'm a little bit confusing about what is the `.status.conditions` we are talking about here :)
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985303318
> should that be part of the kit condition/status then ?
I think it is already happening. We stores conditions of certain traits overridden properties and deprecation notices. It may be polished a little bit (see https://github.com/apache/camel-k/issues/5027) but the logic to store them is already there.
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
lburgazzoli commented on PR #5233:
URL: https://github.com/apache/camel-k/pull/5233#issuecomment-1985293911
>
> Yes. The point for the consume of conditions was more directed to a human observer that wants to understand why, for instance, an Quarkus native Integration is instructing the Kit to use 4GB of memory (which is the sensible default configuration the operator overrides).
should that be part of the kit condition/status then ?
--
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
Re: [PR] fix(trait): revert persisted status [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez merged PR #5233:
URL: https://github.com/apache/camel-k/pull/5233
--
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