feat: demo_mode should work also when no graph api token is set#2185
feat: demo_mode should work also when no graph api token is set#2185
Conversation
alepane21
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
WalkthroughAdds demo-mode-aware handling to config client resolution and polling: allows starting without a graph token or primary client in demo mode, constructs the config poller via options (conditionally including a primary client), and returns a default demo config when no config source or config is found in demo mode. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
router/pkg/controlplane/configpoller/config_poller.go (3)
127-135: Avoid log spam in demo mode when no primary client is configuredThis warning will fire on every poll. Gate it to warn only once (first fetch) and use debug thereafter.
Apply this diff:
- if c.configClient == nil && c.demoMode { - c.logger.Warn("The router is running in demo mode and no config client has been found. Router is using a demo execution config that can be used for testing purposes.") - return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil - } + if c.configClient == nil && c.demoMode { + if c.latestRouterConfigVersion == "" { + c.logger.Warn("The router is running in demo mode and no config client has been found. Using default demo execution config.") + } else { + c.logger.Debug("Demo mode: no config client; using default demo execution config") + } + return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil + }
145-147: Same here: warn once, then downgrade to debugPrevents repeated warnings on every poll when upstream returns not found in demo mode without fallback.
- if c.demoMode && c.fallbackConfigClient == nil && errors.Is(err, ErrConfigNotFound) { - c.logger.Warn("The router is running in demo mode and no config has been found. Router is using a demo execution config that can be used for testing purposes.") - return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil - } + if c.demoMode && c.fallbackConfigClient == nil && errors.Is(err, ErrConfigNotFound) { + if c.latestRouterConfigVersion == "" { + c.logger.Warn("Demo mode: no execution config found. Using default demo config.") + } else { + c.logger.Debug("Demo mode: no execution config found. Using default demo config.") + } + return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil + }
157-159: Log fallback-not-found path for observabilityVisibility helps explain why the router is serving the demo config from fallback.
- if c.demoMode && errors.Is(err, ErrConfigNotFound) { - return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil - } + if c.demoMode && errors.Is(err, ErrConfigNotFound) { + if c.latestRouterConfigVersion == "" { + c.logger.Warn("Demo mode: fallback execution config not found. Using default demo config.") + } else { + c.logger.Debug("Demo mode: fallback execution config not found. Using default demo config.") + } + return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil + }router/core/init_config_poller.go (1)
160-161: Constructor token appears unused in pollerconfigpoller.New takes a token but config_poller.go does not use graphApiToken. Consider removing the parameter in a follow-up to reduce confusion.
Happy to draft a scoped follow-up PR plan if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
router/core/init_config_poller.go(3 hunks)router/pkg/controlplane/configpoller/config_poller.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
router/pkg/controlplane/configpoller/config_poller.go (2)
router/pkg/routerconfig/client.go (2)
Response(11-14)GetDefaultConfig(27-59)router/pkg/config/config.go (1)
Config(985-1059)
router/core/init_config_poller.go (1)
router/pkg/controlplane/configpoller/config_poller.go (7)
Option(15-15)WithLogger(177-181)WithPolling(183-188)WithFallbackClient(196-200)WithDemoMode(202-206)WithClient(190-194)New(46-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
router/pkg/controlplane/configpoller/config_poller.go (1)
17-19: No action required: ErrConfigNotFound compatibility verified Both the S3 client (returnsconfigpoller.ErrConfigNotFoundon “NoSuchKey”) and the CDN client convert their internal 404 sentinel toconfigpoller.ErrConfigNotFound, so allerrors.Is(err, ErrConfigNotFound)checks will succeed.router/core/init_config_poller.go (3)
91-97: Good: allow demo mode without a tokenReturning (nil, nil) here matches the “run without polling” intent.
156-158: Conditional WithClient looks correctOnly attach the primary client when present; avoids nil derefs.
150-156: Verify polling behavior in demo mode without a primary client
Ensure thatconfigPoller.Subscribeactually skips or disables polling whendemoMode==trueandprimaryClient==nil; if it doesn’t, apply the suggested change to set interval and jitter to 0 in that case.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router/pkg/controlplane/configpoller/config_poller.go (1)
132-134: Prefer generic wording; optionally attempt fallback when primary is nil
- Message: "config client" is internal (echoing prior feedback). Suggest "configuration source".
- Optional: If a fallback is configured, consider using it when the primary client is nil (non-demo mode) instead of failing immediately.
Apply:
- if c.configClient == nil { - return nil, errors.New("no config client found") - } + if c.configClient == nil { + if c.fallbackConfigClient != nil { + c.logger.Warn("No primary configuration source configured. Attempting fallback storage") + return (*c.fallbackConfigClient).RouterConfig(ctx, c.latestRouterConfigVersion, c.latestRouterConfigDate) + } + return nil, errors.New("no configuration source configured") + }
🧹 Nitpick comments (2)
router/pkg/controlplane/configpoller/config_poller.go (2)
127-130: Fix log grammar; confirm intended precedence over fallback in demo mode
- Grammar: "a execution" → "an execution".
- Behavior: When
demoModeis true andconfigClientis nil, you return the default config without checking a configured fallback. Is that intentional? If a fallback is provided, should we prefer it over the hardcoded demo config?Apply:
- c.logger.Warn("The router is running in demo mode without a execution configuration source. Using a default demo execution config for testing purposes.") + c.logger.Warn("The router is running in demo mode without an execution configuration source. Using a default demo execution config for testing purposes.")
154-159: Include error details when falling back; log when returning demo config from fallback path
- Add
zap.Error(err)to the fallback attempt log.- When demo mode uses the default config after fallback returns
ErrConfigNotFound, log that decision for parity with the primary path.Apply:
- c.logger.Warn("Failed to retrieve execution config. Attempting with fallback storage") + c.logger.Warn("Failed to retrieve execution config. Attempting with fallback storage", zap.Error(err)) config, err = (*c.fallbackConfigClient).RouterConfig(ctx, c.latestRouterConfigVersion, c.latestRouterConfigDate) - if c.demoMode && errors.Is(err, ErrConfigNotFound) { - return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil - } + if c.demoMode && errors.Is(err, ErrConfigNotFound) { + c.logger.Warn("Fallback storage returned no execution config in demo mode. Using a demo execution config for testing purposes.") + return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
router/pkg/controlplane/configpoller/config_poller.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/pkg/controlplane/configpoller/config_poller.go (2)
router/pkg/routerconfig/client.go (2)
Response(11-14)GetDefaultConfig(27-59)router/pkg/config/config.go (1)
Config(985-1059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
🔇 Additional comments (1)
router/pkg/controlplane/configpoller/config_poller.go (1)
145-147: Fix log grammar
Change the warning message to past tense and simplify. Apply:- c.logger.Warn("The router is running in demo mode and no execution config has been found. Router is using a demo execution config that can be used for testing purposes.") + c.logger.Warn("The router is running in demo mode and no execution config was found. Using a demo execution config for testing purposes.")
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/controlplane/configpoller/config_poller.go (1)
41-44: Fix pointer-to-interface: replace with plain interface to avoid panics and simplify usage.Using *routerconfig.Client (pointer to interface) is a Go anti-pattern and can panic when dereferenced if the underlying interface is nil. Store the interface by value instead and adjust the option and call sites.
@@ type configPoller struct { graphApiToken string logger *zap.Logger latestRouterConfigVersion string latestRouterConfigDate time.Time poller controlplane.Poller pollInterval time.Duration pollJitter time.Duration configClient routerconfig.Client - fallbackConfigClient *routerconfig.Client + fallbackConfigClient routerconfig.Client demoMode bool } @@ -func WithFallbackClient(client *routerconfig.Client) Option { +func WithFallbackClient(client routerconfig.Client) Option { return func(s *configPoller) { s.fallbackConfigClient = client } } @@ - c.logger.Warn("Failed to retrieve execution config. Attempting with fallback storage") - - config, err = (*c.fallbackConfigClient).RouterConfig(ctx, c.latestRouterConfigVersion, c.latestRouterConfigDate) + c.logger.Warn("Failed to retrieve execution config. Attempting with fallback storage") + config, err = c.fallbackConfigClient.RouterConfig(ctx, c.latestRouterConfigVersion, c.latestRouterConfigDate)Also applies to: 196-200, 150-157
🧹 Nitpick comments (2)
router/pkg/controlplane/configpoller/config_poller.go (2)
127-135: Try fallback storage before defaulting to demo config when no primary client.If a fallback client is configured, prefer it over the demo default. This keeps demo mode useful while still honoring a valid fallback source.
- if c.configClient == nil && c.demoMode { - c.logger.Warn("The router is running in demo mode without an execution configuration source, using a demo execution config for testing purposes.") - return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil - } - - if c.configClient == nil { - return nil, errors.New("no execution configuration source found") - } + if c.configClient == nil { + // Prefer fallback storage if available + if c.fallbackConfigClient != nil { + c.logger.Warn("No primary execution configuration source configured; attempting fallback storage.") + cfg, ferr := c.fallbackConfigClient.RouterConfig(ctx, c.latestRouterConfigVersion, c.latestRouterConfigDate) + if ferr == nil { + return cfg, nil + } + if c.demoMode && errors.Is(ferr, ErrConfigNotFound) { + c.logger.Warn("The router is running in demo mode and no execution config has been found, using a demo execution config for testing purposes.") + return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil + } + if !c.demoMode { + return nil, ferr + } + } + if c.demoMode { + c.logger.Warn("The router is running in demo mode without an execution configuration source, using a demo execution config for testing purposes.") + return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil + } + return nil, errors.New("no execution configuration source found") + }
146-148: Add a matching warning log for the fallback ErrConfigNotFound path.You warn on the primary path (Line 146) but not when the fallback yields ErrConfigNotFound. Log for parity and easier troubleshooting.
config, err = c.fallbackConfigClient.RouterConfig(ctx, c.latestRouterConfigVersion, c.latestRouterConfigDate) - if c.demoMode && errors.Is(err, ErrConfigNotFound) { - return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil - } + if c.demoMode && errors.Is(err, ErrConfigNotFound) { + c.logger.Warn("The router is running in demo mode and no execution config has been found in fallback storage, using a demo execution config for testing purposes.") + return &routerconfig.Response{Config: routerconfig.GetDefaultConfig()}, nil + }Also applies to: 156-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
router/pkg/controlplane/configpoller/config_poller.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/pkg/controlplane/configpoller/config_poller.go (2)
router/pkg/routerconfig/client.go (2)
Response(11-14)GetDefaultConfig(27-59)router/pkg/config/config.go (1)
Config(985-1059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
router/pkg/controlplane/configpoller/config_poller.go (1)
141-145: No action required: all routerconfig clients return the configpoller error sentinels directly. The S3 and CDN clients both import and return configpoller.ErrConfigNotModified and configpoller.ErrConfigNotFound, so errors.Is(err, ErrConfig…) will match as intended.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Checklist