Skip to content

Use cfg.NewProvider() in run flags to respect enterprise provider factory#4755

Merged
JAORMX merged 4 commits intomainfrom
fix/run-flags-use-new-provider-for-otel-config
Apr 13, 2026
Merged

Use cfg.NewProvider() in run flags to respect enterprise provider factory#4755
JAORMX merged 4 commits intomainfrom
fix/run-flags-use-new-provider-for-otel-config

Conversation

@reyortiz3
Copy link
Copy Markdown
Contributor

@reyortiz3 reyortiz3 commented Apr 12, 2026

Summary

BuildRunnerConfig and configureMiddlewareAndOptions in run_flags.go were loading application config via cfg.NewDefaultProvider().GetConfig(). NewDefaultProvider always reads the global singleton and never checks a registered ProviderFactory. Any custom provider registered via RegisterProviderFactory — for example, one that merges OTEL config from an external source — would be silently bypassed on every thv run.

  • Switch both call sites to cfg.NewProvider().LoadOrCreateConfig(), which checks registeredFactory first and falls back to the default path when no factory is registered
  • Add TestSetupTelemetryConfiguration_LoadOrCreateConfigPath to document and guard the correct config-loading contract: LoadOrCreateConfig() on a PathProvider reads OTEL settings correctly, confirming the code path the fix enables

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

Does this introduce a user-facing change?

No — deployments with no registered factory fall through to NewDefaultProvider as before. Deployments using a custom ProviderFactory now correctly pick up OTEL and other settings from that provider.

Generated with Claude Code

BuildRunnerConfig and configureMiddlewareAndOptions were loading app
config via cfg.NewDefaultProvider().GetConfig(), which always reads the
global singleton and bypasses any registered ProviderFactory. Enterprise
providers register a factory to merge OTEL config from an external
config-server; by skipping the factory, telemetry and usage-metrics
settings from that source were silently ignored on every `thv run`.

Switch both call sites to cfg.NewProvider().LoadOrCreateConfig(), which
checks registeredFactory first, then falls back to the default path for
non-enterprise deployments.

Add a unit test that exercises the LoadOrCreateConfig() path through a
PathProvider to document and guard the correct config-loading contract.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Apr 12, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.94%. Comparing base (00558da) to head (67c3d8e).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4755   +/-   ##
=======================================
  Coverage   68.93%   68.94%           
=======================================
  Files         517      517           
  Lines       54441    54441           
=======================================
+ Hits        37528    37533    +5     
+ Misses      14044    14041    -3     
+ Partials     2869     2867    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread cmd/thv/app/run_flags.go
Comment thread cmd/thv/app/run_flags.go
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Apr 12, 2026
…call

- Thread appConfig from BuildRunnerConfig into buildRunnerConfig and
  configureMiddlewareAndOptions so config is loaded exactly once per run
  instead of twice; eliminates the double I/O and the split-brain risk if
  the backing store returns different snapshots between calls
- Pass the factory-aware configProvider into setupRuntimeAndValidation so
  NewCLIEnvVarValidator also uses the provider from RegisterProviderFactory
  instead of constructing a NewDefaultProvider that bypasses it

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Apr 12, 2026
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Apr 12, 2026
@reyortiz3
Copy link
Copy Markdown
Contributor Author

@claude can you verify if the comments have been addressed?

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 12, 2026

Claude finished @reyortiz3's task in 45s —— View job


Review comment verification

  • Read current run_flags.go to check comment 1 (double config loading)
  • Read current run_flags.go to check comment 2 (setupRuntimeAndValidation using NewDefaultProvider)
  • Post findings

Both review comments have been fully addressed in the current commits.

Comment 1 — Double config loading (line 699)

Resolved. configureMiddlewareAndOptions now accepts appConfig *cfg.Config as a parameter (line 694) rather than calling cfg.NewProvider().LoadOrCreateConfig() independently. Config is loaded once in BuildRunnerConfig and passed through, eliminating the double-load.

Comment 2 — setupRuntimeAndValidation still using NewDefaultProvider (line 323)

Resolved. setupRuntimeAndValidation now accepts configProvider cfg.Provider as a parameter (line 416) and passes it directly to runner.NewCLIEnvVarValidator(configProvider) (line 427). There are zero remaining calls to cfg.NewDefaultProvider() in run_flags.go.

The fix is consistent across all three call sites now — config is loaded once via cfg.NewProvider() in BuildRunnerConfig, and the resulting provider/config is threaded down to all consumers.

@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels Apr 12, 2026
@JAORMX JAORMX merged commit fae9f0d into main Apr 13, 2026
38 checks passed
@JAORMX JAORMX deleted the fix/run-flags-use-new-provider-for-otel-config branch April 13, 2026 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants