feat(integrations): migrate Datadog, Honeycomb and Coralogix onto the shared setup flow#4191
Merged
Merged
Conversation
Four of the fields in the next migration batch prompt with one today (DD_SITE, HONEYCOMB_DATASET, and both *_API_URLs), which the spec had no way to express. The default is applied inside apply_setup rather than only offered as a prompt prefill. A surface that never prompts — the wizard reusing a stored value, or the agent-driven tool in #4169 filling fields from a conversation — must land on the same credentials as someone pressing enter at the CLI, and that only holds if the flow itself substitutes. A field with a default is therefore never missing, whatever `required` says.
… shared setup flow First batch of #4168. These three share one shape — an API key plus a couple of settings, one registered probe verifier each — so they exercise the flow without dragging in a hard case. Grafana (multi-instance, local stack, alternate auth) and Groundcover (dual env-var fallback) are left for a later batch where they get proper attention. `integrations setup <service>` wrote the integration store and stopped. That reads fine at runtime, since credential resolution checks the store first, but the deploy preflight reads environment variables — so an integration you configured and verified could still block `make deploy` as missing. All three now write store, keyring and .env. The wizard was no better, in ways that were invisible until now: _configure_datadog called sync_env_values({}) and so wrote nothing at all, while _configure_honeycomb and _configure_coralogix wrote their non-secret fields but dropped the API key entirely. Setup now verifies before it persists. Datadog previously saved first, so a typo'd key overwrote a working integration and still printed "Saved"; the smoke test pinning that order is rewritten to pin the new one. A spec's env_var is not derivable from its credential name — base_url is written as HONEYCOMB_API_URL, endpoint as GRAFANA_INSTANCE_URL. Declare the wrong one and the value lands in .env and is silently never read back, with every existing test still green because they mock the writers. Two things close that: _catalog_impl now imports the same config/constants/<vendor> names the specs write, and a round-trip test persists through the real .env writer, reloads it, and requires load_env_integrations to return the same credentials. Also extracts the wizard's prompt-verify-retry loop to configurators/spec_flow.py before it was copied a fourth time, and deletes validate_{datadog,honeycomb,coralogix}_integration — each was a line-for-line duplicate of the client's probe_access() that the registered verifier already calls.
Contributor
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR migrates Datadog, Honeycomb, and Coralogix onto the shared
Confidence Score: 5/5Safe to merge. The three integrations behave correctly on all setup surfaces and no longer silently drop credentials from the keyring and .env. The migration correctly fixes real credential-persistence bugs for all three vendors. The shared apply_setup path enforces verify-before-persist, and the new constants modules make it impossible for the setup spec and catalog reader to silently disagree on env var names. Test coverage is thorough: parametrized unit tests per vendor, a wizard configurator unit test, and an end-to-end round-trip test that persists through the real .env writer and reloads via the catalog. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as opensre integrations setup
participant Wizard as Wizard configurator
participant Flow as apply_setup (setup_flow.py)
participant Verifier as probe_access / verifier
participant Store as Integration store
participant Keyring as sync_env_secret (keyring)
participant Env as sync_env_values (.env)
CLI->>Flow: _run_spec_setup(spec, values)
Wizard->>Flow: configure_from_spec(spec)
Note over Flow: _collect_credentials applies field.default if blank
Flow->>Verifier: spec.verify(setup, credentials)
Verifier-->>Flow: "{status, detail}"
alt verification failed
Flow-->>CLI: "SetupOutcome(ok=False)"
Flow-->>Wizard: "SetupOutcome(ok=False) re-prompt"
else verification passed
Flow->>Keyring: sync_env_secret(env_var, value) for each secret field
Flow->>Env: "sync_env_values({non-secret fields})"
Flow->>Store: upsert_integration(service, credentials)
Flow-->>CLI: "SetupOutcome(ok=True, env_path)"
Flow-->>Wizard: "SetupOutcome(ok=True, env_path)"
end
Reviews (5): Last reviewed commit: "refactor(wizard): name the spec configur..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
CodeQL py/import-and-import-from: both new test modules imported integrations.setup_flow as a module *and* pulled IntegrationSetupSpec out of it. Both rely on the module object for monkeypatch.setattr, so the module import is the one to keep.
Collaborator
Author
|
@greptile review |
Greptile flagged `allow_empty=not field.required` in configure_from_spec as misleading for a required field that carries a default, and suggested `not field.required or bool(field.default)`. Not taken: a defaulted field never returns blank, so claiming "empty is acceptable" for it is less accurate, not more — _prompt_value substitutes the default before it ever consults allow_empty, making the argument dead for those fields either way. The real gap was that nothing said so. Comments now explain the dead branch and the stored-value-beats-default prefill, and tests/cli/wizard/test_spec_flow.py enforces both rather than leaving them to a comment: a blank answer to a defaulted field is accepted, an optional field without a default may be left empty, a stored value is prefilled over the spec default, and a failed verification re-asks instead of dropping the user out of onboarding.
Collaborator
Author
|
@greptile review |
1 similar comment
Collaborator
Author
|
@greptile review |
`spec_flow` sat one word away from `integrations/setup_flow` and said nothing about which layer it was on, which is a confusing pair to leave behind for the remaining 41 integrations. `configurators/spec_configurator.py` matches how its siblings in that package are named, and the docstring now states the split outright: this module collects values from the user, setup_flow decides where they are persisted.
Contributor
|
😭 Clear commit message. Green tests. Kind review. @muddlebee, stop making the rest of us look bad.
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
Collaborator
Author
|
@greptile review |
This was referenced Jul 22, 2026
Closed
Merged
muddlebee
added a commit
that referenced
this pull request
Jul 22, 2026
…Vercel onto the shared setup flow (#4194) * feat(integrations): migrate Groundcover, GitLab, Sentry, PostHog and Vercel onto the shared setup flow Second batch of #4168, and the first that needed no new spec machinery — everything these five required landed with #4191. Two more wizard configurators were dropping credentials on the floor: _configure_vercel called sync_env_values({}) so neither the token nor the team id reached any tier but the store, and _configure_sentry wrote the URL, org and project slugs but never SENTRY_AUTH_TOKEN to the keyring. That is five such bugs across two batches; hand-written per-vendor persistence was wrong more often than it was right. Both are fixed by removing the persistence code rather than correcting it. GitLab was worse: _setup_gitlab prompted for a URL and a token, called upsert_integration, and stopped — no check for a blank token and no probe at all, so a typo produced a stored integration that failed on first use. Three of the five name their env vars differently from the credential they carry (auth_token as GITLAB_ACCESS_TOKEN, organization_slug as SENTRY_ORG_SLUG, base_url as SENTRY_URL), which is the mistake the round-trip test exists to catch; it now covers all nine migrated integrations. The readers — _catalog_impl, integrations/gitlab and integrations/posthog/config — import the same constants the specs write, so the two sides cannot drift. config/constants/sentry.py and posthog.py already existed but hold OpenSRE's *own* telemetry config: the DSN it reports its crashes to and the write-only key for its analytics project. Those are unrelated to what a user supplies to query their own Sentry or PostHog, so both docstrings now say so and the integration names sit under a separate section. Deletes the four wizard validators, whose files contained nothing else. Three were literally build_X_config + validate_X_config, which is what register_validation_verifier already wires up; Vercel's duplicated client.probe_access(). Vercel's probe failure path turned out to be untested, so tests/integrations/vercel/test_client.py gains that coverage rather than losing it with the validator. * style(config): annotate the new env-name constants as Final config/constants/sentry.py and posthog.py annotate every constant they hold, including DEFAULT_SENTRY_BASE_URL added in the same change; the new *_ENV names did not, which reads as though they are variables. The standalone per-vendor env modules (telegram, datadog, gitlab, …) use no Final and stay as they are — they are consistent with each other and with the telegram precedent. Only the two files that mix styles change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #4168.
Describe the changes you have made in this PR -
First migration batch onto the shared setup flow that #4170 introduced: Datadog, Honeycomb, Coralogix. Three near-identical shapes (API key + a couple of settings, one registered probe verifier each), picked so the batch exercises the flow without dragging in a hard case. Grafana (multi-instance, local stack, alternate auth) and Groundcover (dual env-var fallback) are deliberately left for a later batch.
What each integration gains
opensre integrations setup <service>previously wrote the integration store and stopped. That reads fine at runtime — credential resolution checks the store first — but the deploy preflight reads environment variables, so an integration you configured and verified could still blockmake deployas "missing". All three now write store + keyring +.env.Two bugs on the wizard side turned up while migrating, both now fixed by construction:
_configure_datadogsync_env_values({})—DD_API_KEY,DD_APP_KEYandDD_SITEreached no tier but the store_configure_honeycombHONEYCOMB_DATASET+HONEYCOMB_API_URL, neverHONEYCOMB_API_KEY_configure_coralogixCORALOGIX_API_KEYBehavior change worth calling out: setup now verifies before it persists. Previously Datadog saved and then verified, so a typo'd key overwrote a working integration and the command still printed
Saved.tests/cli/test_smoke.pywas pinning that old order and has been rewritten to pin the new one.New spec capability:
SetupField.defaultFour of the six migrated fields prompt with a default (
DD_SITE,HONEYCOMB_DATASET, both*_API_URLs). The default is applied insideapply_setup, not only offered as a prompt prefill — otherwise a surface that never prompts (the agent-driven tool in #4169) would land on different credentials than someone pressing enter at the CLI.Guard against the failure mode this batch is most exposed to
A spec's
env_varis not derivable from its credential name —base_urlis written asHONEYCOMB_API_URL,endpointasGRAFANA_INSTANCE_URL. Declare the wrong one and the value lands in.envand is silently never read back; every existing unit test still passes, because they mock the writers.Two things close that hole:
integrations/_catalog_impl.py— the reader — now imports the sameconfig/constants/<vendor>.pynames the specs write, so the two sides cannot drift for these vendors.tests/integrations/test_setup_spec_env_round_trip.pypersists through the real.envwriter, reloads the file into the environment, and requiresload_env_integrations()to hand back the same credentials. It covers Telegram too.I mutation-tested that guard rather than trusting a green run — changing Honeycomb's
env_varto the plausible-but-wrongHONEYCOMB_BASE_URLfails it:Deduplication
surfaces/cli/wizard/configurators/spec_flow.py— the wizard's prompt-verify-retry loop, which was about to be copy-pasted a fourth time. Telegram is folded onto it, so_configure_telegramdrops from 33 lines to 14.validate_datadog_integration,validate_honeycomb_integration,validate_coralogix_integrationdeleted. Each was a line-for-line duplicate of the correspondingclient.probe_access()that the registered verifier already calls — same calls, same checks, only the message wording differed.probe_accessis already covered intests/integrations/test_integration_probes.pyand the per-client suites, so no coverage is lost.Demo/Screenshot for feature changes and bug fixes -
Verify-before-persist, run against a sandboxed
HOMEwith placeholder keys:Before this change the same run would have written the store and printed
Saved.Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The problem is that three surfaces configure integrations and they disagreed about where credentials go, so whether an integration blocked
make deploydepended only on which command you happened to use. #4170 builtapply_setupand migrated Telegram as the reference; this PR is the first batch of the remaining 44.The alternative I rejected was making
upsert_integrationmirror everything to the environment automatically. That cannot work: the store key isapi_keywhile the env var isDD_API_KEYorHONEYCOMB_API_KEY, so a per-integration name map is unavoidable — and that map is the spec.I also considered a bigger first batch (five integrations, adding Grafana and Groundcover). I cut it to three because Grafana carries multi-instance parsing, a local Docker stack, and username/password auth, and folding that into a batch PR would have hidden real decisions inside mechanical churn.
Key pieces:
integrations/<vendor>/setup.py— declares what the integration needs; no logic beyond Telegram-style resolution, which none of these three require.SetupField.default— the one new spec capability, applied in the flow rather than at the prompt so every surface agrees.spec_flow.configure_from_spec— the wizard's collection loop, extracted before it got copied a fourth time.test_setup_spec_env_round_trip.py— the only test that can catch a wrongenv_var, since the others mock the writers.Edge cases covered: blank required field fails on that field rather than after all prompts; a field with a default is never "missing"; a submitted value beats the default; failed verification persists nothing on any tier; clearing an optional field clears every tier rather than leaving a stale
.envline that would keep resolving.Checklist before requesting a review