feat(integrations): migrate SMTP, WhatsApp and Grafana Tempo onto the shared setup flow#4197
Conversation
… shared setup flow Fourth migration batch (#4168): three more integrations onto IntegrationSetupSpec, none of them with name mismatches or dropped credentials this time — Tempo's wizard configurator already persisted correctly to all three tiers. Also hardens tests/integrations/test_setup_spec_env_round_trip.py: the env round-trip fixture only ever added values, so a developer's real local .env could let a wrong env_var still "resolve" from an unrelated pre-existing value with the same name (found via WhatsApp/Twilio's shared TWILIO_ACCOUNT_SID/TWILIO_WHATSAPP_FROM). The fixture now wipes the environment before restoring only what apply_setup actually wrote, so the guard holds regardless of what's already in a contributor's .env.
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 is the fourth batch of the shared-setup-flow migration (#4168), moving SMTP, WhatsApp, and Grafana Tempo onto the shared
Confidence Score: 5/5Safe to merge; the migration follows the established batch pattern with no dropped credentials, correct env var routing, and end-to-end round-trip test coverage for all three new integrations. All three integrations convert cleanly — field names, env var names, default values, and secret flags each match the old hand-rolled handlers. The Tempo wizard configurator removal is fully backed by the generic spec-configurator path, and validate_tempo_integration has no remaining consumers. The round-trip test fixture is now tighter: it wipes ambient env vars before asserting, and guards against a None catalog response before comparing values. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph CLI["integrations/cli.py"]
S1["_setup_smtp()"]
S2["_setup_whatsapp()"]
S3["_setup_tempo()"]
end
subgraph Specs["integrations/*/setup.py"]
SP1["SMTP_SETUP"]
SP2["WHATSAPP_SETUP"]
SP3["TEMPO_SETUP"]
end
subgraph Flow["integrations/setup_flow.py"]
AF["apply_setup()"]
PE["_persist_env()"]
end
subgraph Storage["Persistence Tiers"]
KR["Keyring"]
EF[".env file"]
ST["Integration Store"]
end
S1 --> SP1
S2 --> SP2
S3 --> SP3
SP1 & SP2 & SP3 --> AF
AF --> PE
PE --> KR
PE --> EF
AF --> ST
Reviews (2): Last reviewed commit: "test(setup-flow): reject missing catalog..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ip wipe Greptile flagged on #4197: wiping the entire environment before restoring only what apply_setup wrote could break in the future if resolution ever needs a system var (HOME, PATH, ...). Their suggested narrower fix — clearing only each spec's currently-declared env_var names — would not have caught the bug this wipe exists for: a *wrong* env_var means the correct name is exactly the one missing from that set, so a stray real value under the correct name would still leak through uncleared. Keep the full wipe (it's what makes a wrong name unable to hide behind a coincidentally-set real value), but carve out a short, explicit allowlist of plumbing vars nothing's credential name is ever built from.
|
Good catch on the collateral risk, but the suggested narrower fix (clear only each spec's currently-declared Kept the full wipe, but carved out a short explicit allowlist ( |
muddlebee
added
the
automerge
|
@greptile review |
1 participant
Part of #4168. Follows #4191 (batch 1), #4194 (batch 2), and #4196 (batch 3, still open).
Describe the changes you have made in this PR -
Fourth migration batch onto the shared setup flow: SMTP, WhatsApp, Grafana Tempo. All three are "N fields + a registered verifier" with no branching prompts, either/or groups, or env-var name mismatches.
No dropped-credential bugs this time
Tempo's wizard configurator (
_configure_tempo) already wrote to the store, keyring and.envcorrectly — it's the first one migrated in this whole effort that had nothing wrong with it. Migrating it anyway removes the second, hand-maintained persistence implementation, which is the actual point (two implementations that happen to agree today can still drift tomorrow). SMTP and WhatsApp never had a wizard configurator at all, onlyopensre integrations setup <service>.A cross-field validator already existed and needed no changes
SMTP's old CLI handler had a
_diecheck beyond simple required/optional:if bool(username) != bool(password): _die(...). That looked like exactly the "bespoke validation" #4168 warns doesn't drop straight into a spec — except it turns outSMTPIntegrationConfig(integrations/config_models.py) already has this as a pydanticmodel_validator, soverify_smtp(the registered verifier) already enforces it. Nothing to add; flagging it here because I initially assumed it needed a spec feature and it didn't.Twilio's account credentials are genuinely shared
account_sid/auth_tokenare one Twilio account, not per-channel —TWILIO_ACCOUNT_SID/TWILIO_AUTH_TOKENare the same two env vars whatsapp and (eventually) Twilio SMS both read.config/constants/twilio.pyholds them under the Twilio name for that reason, documented in both the constants module andintegrations/whatsapp/setup.py's docstring, so this isn't mistaken for a naming bug later.Found and fixed a real gap in the round-trip test itself
tests/integrations/test_setup_spec_env_round_trip.pypersists through the real.envwriter and asserts the catalog reads the same values back — but its fixture only ever added env vars, it never cleared what was already there.tests/conftest.pyloads a contributor's real local.envinto the test process for the whole session, so a spec with a wrongenv_varcould still "pass" if the correct name happened to already be sitting in that real.envfrom unrelated local setup. I hit this for real: mutation-testing WhatsApp'sfrom_numberagainst a deliberately wrong env name still passed, because my own machine's.envalready had the correctTWILIO_WHATSAPP_FROMset from something else.Fixed by wiping the environment before restoring only what
apply_setupactually wrote (re-settingOPENSRE_DISABLE_KEYRING=1afterward so an unset field still misses cleanly instead of touching a real OS keyring). Re-ran the WhatsApp mutation test after the fix — it now fails as expected, and the full 7-integration suite still passes clean. This hardens the guard for every integration already migrated, not just this batch.Demo/Screenshot for feature changes and bug fixes -
Env round-trip guard catching a deliberately wrong env var, sandboxed:
Before the fixture fix, this same mutation silently passed.
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 the last three batches — three surfaces configured integrations and disagreed about where credentials land. This batch is smaller (3 vs. the previous 9) because I spent real time verifying each candidate wasn't secretly like RDS or ServiceNow (see #4196's PR body) before committing to it: checked
SMTPIntegrationConfig/WhatsAppConfig/TempoConfigfield names against their CLI handlers, checked env var names againstintegrations/_catalog_impl.py's readers, and checked for wizard configurators before assuming there wasn't one to migrate.Key pieces:
integrations/{smtp,whatsapp,tempo}/setup.py— declarative field lists; no logic.config/constants/{smtp,tempo,twilio}.py— env names, imported by both writer and reader.configure_from_spec; its explanatory intro banner (bearer-token-XOR-basic-auth guidance) is preserved via theintro=parameter.Edge cases covered: SMTP's
portround-trips as a string through.env/keyring but is read back as anint(pydantic coercion) — the round-trip test now comparesstr(...)on both sides so that legitimate type normalization doesn't look like a persistence bug; the cross-field XOR validator on SMTP's username/password fires through the normal failed-verification path with nothing persisted; WhatsApp's optionaldefault_toclears every tier when blanked, matching every other migrated integration.Checklist before requesting a review