feat(integrations): migrate incident.io, Tracer, MongoDB Atlas, SigNoz, Jenkins, PagerDuty, Dagster, Temporal and Helm onto the shared setup flow#4196
Conversation
…z, Jenkins, PagerDuty, Dagster, Temporal and Helm onto the shared setup flow Third migration batch (#4168): nine integrations onto IntegrationSetupSpec. No name mismatches this time, but two more wizard configurators (PagerDuty, and the same class of bug already seen elsewhere) were dropping credentials before persistence — both configurators now delegate to configure_from_spec like every other migrated integration. Also fixes a false-positive: MONGODB_ATLAS_PUBLIC_KEY's terminal token ("key") routed it to the keyring even though it is a paired public identifier, not a secret — added to the same env_file.py allowlist that already covers DISCORD_PUBLIC_KEY.
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. |
Greptile SummaryThis PR migrates nine more integrations (incident.io, Tracer, MongoDB Atlas, SigNoz, Jenkins, PagerDuty, Dagster, Temporal, Helm) onto the shared
Confidence Score: 5/5Safe to merge — the nine integrations land on the existing shared setup flow without new logic paths, and the two bugs fixed (PagerDuty credential drop, MongoDB Atlas public-key mis-routing) are both covered by round-trip tests that would have caught any regression. The change is mechanical migration backed by a layered test suite: per-spec CLI tests (prompt → store), end-to-end round-trip tests (write → catalog read-back), wizard flow tests (retry/skip secret), and a new unit test for blank-value keyring deletion. Each new spec is cross-checked against the catalog's actual env-var reader rather than a mock, so the tests would catch naming drift between the writer and reader sides. No new conditional logic was introduced. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[opensre integrations setup service] --> B[_setup_service in cli.py]
B --> C[_run_spec_setup with SPEC]
C --> D[IntegrationSetupSpec with fields and verify fn]
D --> E[apply_setup]
E --> F{_collect_credentials applies defaults}
F -->|required field blank| G[SetupOutcome ok=False nothing persisted]
F -->|all fields present| H{_verify calls spec.verify}
H -->|failed| G
H -->|passed| I[_persist_env iterates fields with env_var]
I --> J{is_sensitive_env_key check}
J -->|Yes| K[sync_env_secret to keyring]
J -->|No| L[sync_env_values to .env]
K --> M[upsert_integration to store]
L --> M
M --> N[SetupOutcome ok=True]
Reviews (2): Last reviewed commit: "merge(main): resolve conflicts with setu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| service="dagster", | ||
| fields=( | ||
| SetupField( | ||
| name=ENDPOINT_FIELD, | ||
| label="Dagster GraphQL endpoint", | ||
| prompt=( | ||
| "Dagster GraphQL endpoint " | ||
| "(e.g. http://localhost:3000 or https://<org>.dagster.plus/<deployment>)" | ||
| ), | ||
| env_var=DAGSTER_ENDPOINT_ENV, | ||
| default="http://localhost:3000", | ||
| ), | ||
| SetupField( | ||
| name=API_TOKEN_FIELD, | ||
| label="Dagster Cloud API token", |
There was a problem hiding this comment.
The wizard configurator always prefilled http://localhost:3000 when no stored credential existed, so the default here is consistent for the wizard path. However, the old _setup_dagster CLI handler rejected a blank endpoint immediately with _die("endpoint is required."), while _run_spec_setup now applies the spec's default and proceeds to verify http://localhost:3000. A Dagster Cloud user who accidentally presses Enter at the endpoint prompt will now receive a connection-refused error from the verifier instead of the earlier "endpoint is required" message. That's still a failure, but the failure reason is less obvious. Consider either keeping the field required=True with no default (restore the old "required" guard) or making it required=False with the current default so the intent is unambiguous.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Greptile flagged on #4196: apply_setup calls sync_env_secret for every secret field on every save (submitted or not), relying on a blank value deleting the old keyring entry rather than being skipped — but nothing exercised that path directly, only implied it through a mocked wizard test. Add a real-keyring (MemoryKeyring) test for it.
|
Added a direct test: |
Keep both migration batches' constants, CLI setup cases, and env round-trip coverage. Drop wizard health validators deleted on either side (gitlab/sentry/posthog/vercel and dagster/pagerduty/incident_io).
|
@greptile review |
muddlebee
added
the
automerge
Waiting on Greptile Review strands labeled PRs after the last Actions workflow_run retry, since Greptile is external and never re-triggers Auto-merge. Treat it like vale-spellcheck; review stays a human gate.
|
🛸 Aliens watching our repo just upgraded @muddlebee's threat level to: do not engage — too competent. 👽
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
1 participant
Part of #4168. Follows #4191 (batch 1) and #4194 (batch 2, still open).
Describe the changes you have made in this PR -
Third migration batch onto the shared setup flow: incident.io, Tracer, MongoDB Atlas, SigNoz, Jenkins, PagerDuty, Dagster, Temporal, Helm. All nine were "N fields + a registered verifier" with no branching prompts and — unlike batch 2 — no env-var name mismatches, so this batch needed no new spec features.
Two more wizard configurators were silently dropping credentials
_configure_pagerdutysync_env_values({})— neitherPAGERDUTY_API_KEYnorPAGERDUTY_BASE_URLreached any tier but the store_configure_betterstack*sync_env_values({}), all four fields store-only*Better Stack itself is not migrated in this batch (see below) — its wizard bug is left in place and flagged for whoever picks it up.
That makes seven such bugs across three batches (Datadog, Honeycomb, Coralogix, Sentry, Vercel, and now PagerDuty).
_configure_incident_io,_configure_jenkinsand_configure_dagsterwere already correct — migrated anyway so persistence has one implementation instead of two that happen to agree today.A false-positive secret classification, caught by the round-trip test
config.env_file.is_sensitive_env_key()routes any env var whose name ends inkeyto the keyring.MONGODB_ATLAS_PUBLIC_KEYmatched that even though it's a paired public identifier (used alongside a private key for Digest Auth), not a secret on its own —tests/integrations/test_cli_spec_setup.py's keyring/env split assertion caught this immediately (it expected the public key in.env, got it in the keyring instead). Fixed the same wayDISCORD_PUBLIC_KEYalready was: added to the_NON_SECRET_ENV_KEYSallowlist inconfig/env_file.py, with a test case intests/cli/wizard/test_env_sync.py.Tracer needed its own round-trip check
integrations._catalog_impl.load_env_integrations()is the function every other spec's env round-trip goes through — but Tracer's env-only discovery isn't in it at all. It's a top-level fallback insideresolve_effective_integrations()instead (JWT_TOKENchecked directly, no per-vendor block in the env loader).tests/integrations/test_setup_spec_env_round_trip.pynow special-cases Tracer to assert againstresolve_effective_integrations()rather thanload_env_integrations()— same guarantee, correct target.Helm's env-only discovery has a separate opt-in gate
load_env_integrations()only surfaces Helm whenOSRE_HELM_INTEGRATIONis truthy — a deliberate manual switch (Helm shells out to a local binary) unrelated to any credential field. Not a bug, but the round-trip test needed to set that flag alongside the persisted values to exercise the discovery path at all; documented inintegrations/helm/setup.py's docstring so the next person doesn't mistake it for a missedSetupField.Betterstack and RDS were deliberately left out
betterstack'ssourcesfield is a list downstream (betterstack_extract_paramsdoeslist(bs.get("sources", []))), but every otherSetupFieldis a plain string — persisting the field as-is would silently corrupt it (list("a,b")iterates characters). Needs a small fix tointegrations/betterstack/__init__.pyto normalize either shape, which is a real code change beyond wiring a spec, not a mechanical migration.rds's current_setup_rdsCLI handler prompts forhost/port/database/username/password, butintegrations/rds/__init__.py's actual config (classify,rds_is_available,rds_extract_params) readsdb_instance_identifier/region— fields the handler never asks for.opensre integrations setup rdshas likely never produced a usable RDS integration. Worth its own PR rather than folding a behavior fix into a batch of otherwise-mechanical migrations.Both are called out on #4168 for whoever picks them up next.
Demo/Screenshot for feature changes and bug fixes -
PagerDuty verify-before-persist, sandboxed
HOME, bad key:Before this change,
_configure_pagerduty(the wizard path) would have saved the key to the store even after a failed probe elsewhere reported "ok" — the store and the failed check were never the same call.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:
Same problem as #4191/#4194 — three surfaces configured integrations and disagreed about where credentials land, so whether an integration blocked
make deploydepended on which command you used. This batch moves nine more onto the one flow.I picked these nine specifically because none of them have branching prompts, either/or field groups, or post-save side effects — the three gaps #4168 calls out as needing new spec features before they can move. Grafana, the databases, GitHub, AWS, Discord, Slack and Rocket.Chat are still blocked on those; Better Stack and RDS turned out to have their own vendor-specific issues once I looked closely (see above), so they're deferred rather than folded in here.
Key pieces:
integrations/<vendor>/setup.py× 9 — declarative field lists; no logic.config/constants/<vendor>.py× 9 — new env-name modules, imported by both the writer (spec) and the reader (load_env_integrations/resolve_effective_integrations), so the two sides can't drift silently.configure_from_spec; Tracer, MongoDB Atlas, SigNoz, Temporal and Helm never had a wizard path (onlyopensre integrations setup <service>), so there was nothing to migrate there.Edge cases covered by the extended parametrized suites: blank required field fails on that field rather than after every prompt (skipped only for Helm and Dagster, where every field now has a default — verified
next()would raiseStopIterationthere before excluding them); defaulted fields (PagerDuty's base URL, Temporal's namespace, Helm's binary path) accept a bare enter; optional fields (incident.io's base URL, Dagster's API token, Temporal's API key, Helm's context/kubeconfig/namespace) may be left blank and clear every tier when blanked; failed verification persists nothing anywhere.One mutation test to confirm the round-trip guard actually catches drift: pointed Temporal's namespace at a wrong env name and confirmed the suite fails with the exact mismatch, then reverted.
Checklist before requesting a review