Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(trait): Move addon Telemetry to traits #5814

Merged
merged 3 commits into from
Sep 4, 2024

Conversation

gansheer
Copy link
Contributor

@gansheer gansheer commented Sep 3, 2024

Ref #5787

  • Transform Telemetry addon to trait
  • Ensure kamel CLI creates new integrations with telemetry as trait
  • keep retrocompatibility with addon spec:
  traits:
    addons:
      telemetry:
        enabled: true
        endpoint: http://opentelemetrycollector.otlp:4317

Release Note

feat(trait): Move addon Telemetry to traits

@gansheer gansheer force-pushed the feat/5787_telemetry_addon_trait branch from 1fd750b to 84750ad Compare September 3, 2024 17:50
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think you need to check how the kamel run would convert to an addons spec or to an trait spec.

@gansheer gansheer force-pushed the feat/5787_telemetry_addon_trait branch 2 times, most recently from 5fe217a to f16e0cc Compare September 4, 2024 09:20
@@ -438,6 +438,7 @@ func TestConfigureTraits(t *testing.T) {
"--trait", "affinity.pod-affinity=false",
"--trait", "environment.container-meta=false",
"--trait", "prometheus.pod-monitor=false",
"--trait", "telemetry.auto=true",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@squakez That was the point of this change to test is the run command really creates a trait and not an addon.

@gansheer gansheer force-pushed the feat/5787_telemetry_addon_trait branch from f16e0cc to 521bd4e Compare September 4, 2024 09:36
@gansheer gansheer marked this pull request as ready for review September 4, 2024 09:42
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I miss is some deprecation notice in the Integration which is using the addons. Ideally, when executing the trait logic, it should be reported as an Integration condition as it happens for instance here:

condition = newOrAppend(condition, "Spectrum publishing strategy is deprecated and may be removed in future releases. Make sure to use any supported publishing strategy instead.")

@gansheer gansheer force-pushed the feat/5787_telemetry_addon_trait branch 2 times, most recently from 0df7f8d to 73ca2f5 Compare September 4, 2024 13:38
return true, nil, nil
var condition *TraitCondition

if _, isAddon := e.Integration.Spec.Traits.Addons["telemetry"]; isAddon {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we also need to add a // Deprecated comment as it's the way I usually scan code for deprecations to be eventually removed.

@gansheer gansheer force-pushed the feat/5787_telemetry_addon_trait branch from 73ca2f5 to 00c253f Compare September 4, 2024 13:41
@gansheer gansheer merged commit cbaa59c into apache:main Sep 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants