-
Notifications
You must be signed in to change notification settings - Fork 156
Refactor CRD Telemetry Config Conversion for Reusability #2908
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2908 +/- ##
==========================================
- Coverage 56.39% 56.26% -0.14%
==========================================
Files 322 323 +1
Lines 31622 31640 +18
==========================================
- Hits 17834 17801 -33
- Misses 12257 12308 +51
Partials 1531 1531 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
9419346 to
0899abd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors telemetry configuration conversion logic to improve code reusability. It extracts the conversion logic from WithTelemetryConfig into standalone functions (MaybeMakeConfig and ConvertTelemetryConfig) that can be used independently, particularly for integration with the vMCP CRD in PR #2906.
Key changes:
- Created
telemetry.MaybeMakeConfig()to handle the core telemetry config construction logic from individual parameters - Renamed
WithTelemetryConfig()toWithTelemetryConfigFromFlags()and created a newWithTelemetryConfig()that accepts a pre-built config - Introduced
spectoconfigpackage withConvertTelemetryConfig()to convert CRD TelemetryConfig to telemetry.Config
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/telemetry/config.go |
Added MaybeMakeConfig() function to create telemetry configuration from individual parameters |
pkg/runner/config_builder.go |
Renamed WithTelemetryConfig to WithTelemetryConfigFromFlags, created new WithTelemetryConfig that accepts config directly |
cmd/thv-operator/pkg/spectoconfig/telemetry.go |
New package/file containing ConvertTelemetryConfig() to convert CRD types to telemetry.Config |
cmd/thv-operator/pkg/runconfig/telemetry.go |
Simplified to use spectoconfig.ConvertTelemetryConfig() instead of inline conversion logic |
pkg/runner/config_test.go |
Updated all test calls from WithTelemetryConfig to WithTelemetryConfigFromFlags |
pkg/api/v1/workload_service.go |
Updated function call to use WithTelemetryConfigFromFlags |
cmd/thv/app/run_flags.go |
Updated function call to use WithTelemetryConfigFromFlags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor CRD Telmetry Config Conversion for Reusability - Moved the telemetry.Config construction logic that was within WithTelemetryConfig to telemetry.MaybeMakeConfig. - Renamed WithTelemetryConfig to WithTelemetryConfigFlags which behaves just as before. - Created WithTelemetryConfig which just assign the given telemetry config. - Created spectoconfig/telemetry.go (pronounced "Spec To Config" other names welcome) which contains ConvertTelemetryConfig. ConvertTelemetryConfig is exactly the same as AddTelemetryConfigOptions minus creating the option. Signed-off-by: Jeremy Drouillard <jeremy@stacklok.com>
Summary
This pull request refactors the existing logic of
AddTelemetryConfigOptionsso the conversion frommcpv1alpha1.TelemetryConfigtotelemetry.Configis reusable without having to create aRunConfigBuilderOption.I'd like to reuse just this conversion logic within #2906, so the vMCP CRD uses the same exact telemetry type as the MCP Proxy, etc.
This PR isn't small but it's mostly just moving code to places that make it more composable.
Implementation Details
telemetry.Configconstruction logic that was withinWithTelemetryConfigtotelemetry.MaybeMakeConfig.WithTelemetryConfigtoWithTelemetryConfigFlagswhich behaves just as before.WithTelemetryConfigwhich just assign the given telemetry config.spectoconfig/telemetry.go(pronounced "Spec To Config" other names welcome) which containsConvertTelemetryConfig.ConvertTelemetryConfigis exactly the same asAddTelemetryConfigOptionsminus creating the option.