feat(experimentation): publish warehouse connections to Redis and apply delivery status - #8551
gagantrivedi wants to merge 6 commits into
Conversation
…ly delivery status The warehouse-delivery service reads each environment's connection from experimentation:environment_warehouses:<client key> and leaves each connection's outcome in the experimentation:warehouse_delivery_status hash. sync_environment_ingestion now writes the connection, with the credentials as the same Fernet ciphertext the database holds, before the destination so the service never sees an event it cannot place, and removes it after. A one-minute task copies the outcomes onto WarehouseConnection.status and status_detail. The update hook also fires on config and credentials changes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughThe change moves Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to Failed or pending warehouse connections can still receive routed events, causing delivery failures. This should be fixed before merge. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8551 +/- ##
========================================
Coverage 98.81% 98.81%
========================================
Files 1620 1628 +8
Lines 66186 66394 +208
========================================
+ Hits 65401 65609 +208
Misses 785 785 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… module encrypt_warehouse_credentials and decrypt_warehouse_credentials live in core/warehouse_credentials.py, named for the secret they are bound to. EncryptedJSONField and the ingestion sync service both call them.
…ryption into the app Both are only about a warehouse connection's credentials. core/fields.py keeps NoSSRFURLField alone. Migration 0010 now imports the field from its new module, so the migration state matches the model and no new migration is needed.
…t from the ingestion one warehouse_delivery_sync_service.py holds what the warehouse-delivery service reads and writes: publish_warehouse_connection, remove_warehouse_connection, pop_warehouse_delivery_statuses. ingestion_sync_service.py is back to keys and destinations for the ingestion server. Both use the client in ingestion_redis.py. The old warehouse_delivery_service.py, which verifies a connection when it is saved, is renamed warehouse_verification_service.py.
…ying them into Postgres The connection list and detail views read the delivery service's latest outcome for each verified external connection from Redis with one HMGET and, when it failed, show errored with the reason. Nothing is saved; a connection that failed verification keeps that result. Redis being down or unconfigured falls back to the stored status. The one-minute apply_warehouse_delivery_statuses task and the Lua pop go away, and removing a connection also forgets its outcome so nothing stale shows.
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20510 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #20510 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20510 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)Details
✅ oss · depot-ubuntu-latest-16 — run #20510 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)Details
|
Visual Regression19 screenshots compared. See report for details. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 0b14d203-44bd-4373-95b3-191001a70348
📒 Files selected for processing (26)
api/core/fields.pyapi/experimentation/dataclasses.pyapi/experimentation/fields.pyapi/experimentation/ingestion_redis.pyapi/experimentation/ingestion_sync_service.pyapi/experimentation/migrations/0010_warehouse_connection_credentials_and_status_detail.pyapi/experimentation/models.pyapi/experimentation/services.pyapi/experimentation/tasks.pyapi/experimentation/views.pyapi/experimentation/warehouse_credentials.pyapi/experimentation/warehouse_delivery_sync_service.pyapi/experimentation/warehouse_verification_service.pyapi/tests/unit/core/test_fields.pyapi/tests/unit/experimentation/conftest.pyapi/tests/unit/experimentation/test_fields.pyapi/tests/unit/experimentation/test_ingestion_redis.pyapi/tests/unit/experimentation/test_ingestion_sync_service.pyapi/tests/unit/experimentation/test_models.pyapi/tests/unit/experimentation/test_services.pyapi/tests/unit/experimentation/test_tasks.pyapi/tests/unit/experimentation/test_views.pyapi/tests/unit/experimentation/test_warehouse_credentials.pyapi/tests/unit/experimentation/test_warehouse_delivery_sync_service.pyapi/tests/unit/experimentation/test_warehouse_verification_service.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.md
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| environment.api_key, connection_ids=[connection.id] | ||
| ) | ||
| else: | ||
| warehouse_delivery_sync_service.publish_warehouse_connection( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,120p' api/experimentation/tasks.py
sed -n '50,110p' api/experimentation/models.py
sed -n '1360,1460p' api/experimentation/services.py
rg -n 'sync_environment_ingestion|verify_clickhouse_connection|WarehouseConnection.Status|status =|CONNECTED|ERRORED' api/experimentation api/tests/unit/experimentation/test_tasks.py api/tests/unit/experimentation/test_models.py api/tests/unit/experimentation/test_services.py api/tests/unit/experimentation/test_views.pyRepository: Flagsmith/flagsmith
Length of output: 20650
Gate external routing on verification status.
sync_environment_ingestion publishes every non-Flagsmith connection and enables the external destination without checking connection.status. The create and configuration hooks schedule this task before verification. verify_clickhouse_connection saves ERRORED through save(update_fields=["status", "status_detail"]), but those fields do not trigger WarehouseConnection.sync_to_ingestion. A queued task can therefore publish a pending or errored ClickHouse connection, and a failed verification does not remove an already-published destination.
Publish only verified connections. Remove the delivery document and ingestion destination when a ClickHouse connection is pending or errored. Schedule synchronisation when verification changes the persisted status.
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to Flagsmith/flagsmith-analytics-pipeline#29
The warehouse-delivery service reads external connections from the ingestion Redis and leaves each connection's outcome there; this is the API's side of that contract.
sync_environment_ingestionpublishes the connection toexperimentation:environment_warehouses:<client key>before routing events to the external topic, and removes it after unrouting, so the service never sees an event it cannot place. Credentials go in as the same Fernet ciphertext the database holds.experimentation:warehouse_delivery_statushash with oneHMGETand, when it failed, showerroredwith the reason. Nothing is saved, and a connection that failed verification keeps that result. If the ingestion Redis is down or not configured, the stored status is shown.configandcredentialschanges, so a rotated password reaches Redis.Deploy before the service starts, or it commits past the backlog with nothing to deliver to. The service must be given the same
WAREHOUSE_CREDENTIALS_SECRETthe API task definitions already set.How did you test this code?
Unit tests for the service functions, the task and the hook;
make lintandmake typecheckpass; events catalogue regenerated.