Add optional country field to run and EDLA schemas#96
Add optional country field to run and EDLA schemas#96
Conversation
WalkthroughA new optional "country" field (string type) is added to topic schemas (dlchange and runs), with corresponding updates to the PostgreSQL writer to handle the field during database inserts, and test expectations adjusted to account for the additional parameter. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
- Added country field to conf/topic_runs.json (jobs.items.properties) - Added country field to conf/topic_dlchange.json (root properties) - Updated postgres_edla_write to include country column (after timestamp_event) - Updated postgres_run_write to include country column in jobs table (after catalog_id) - Updated tests to validate new country field with empty string default - All tests passing, black/pylint/mypy checks passing Co-authored-by: tmikula-dev <72911271+tmikula-dev@users.noreply.github.com>
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/writers/test_writer_postgres.py (2)
41-66: Consider adding test coverage for country value insertion.The test verifies the default empty string behavior when country is absent, but there's no test that provides an actual country value (e.g., "US" or "ZA") to verify it's correctly inserted.
Add a test case with country provided:
def test_postgres_edla_write_with_country(): cur = MockCursor() message = { "event_id": "e3", "tenant_id": "t3", "source_app": "app", "source_app_version": "1.0.0", "environment": "dev", "timestamp_event": 333, "country": "ZA", "catalog_id": "db.tbl", "operation": "append", "format": "parquet", } writer_postgres.postgres_edla_write(cur, "table_a", message) _sql, params = cur.executions[0] assert params[6] == "ZA" # country value provided
91-122: Test coverage: Consider verifying country value insertion for jobs.Similar to the dlchange test, this verifies the default empty string but doesn't test when country is actually provided in the jobs array.
Consider adding a job with country value:
"jobs": [ { "catalog_id": "c1", "country": "ZA", "status": "succeeded", "timestamp_start": 1100, "timestamp_end": 1200 }, ]Then verify
assert job1_params[2] == "ZA".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
conf/topic_dlchange.json(1 hunks)conf/topic_runs.json(1 hunks)src/writers/writer_postgres.py(5 hunks)tests/writers/test_writer_postgres.py(3 hunks)
🔇 Additional comments (5)
conf/topic_runs.json (1)
46-49: LGTM! Optional country field added correctly.The country field is properly defined as an optional string with a clear description. It's correctly excluded from the required array, maintaining backward compatibility.
conf/topic_dlchange.json (1)
28-31: LGTM! Schema consistency maintained.The country field definition matches the one in topic_runs.json, ensuring consistency across schemas. Proper placement and optional nature maintained.
src/writers/writer_postgres.py (2)
187-222: Verify jobs table schema includes country column.The jobs table INSERT now includes the country column. Ensure that the
public_cps_za_runs_jobstable has been updated with the country column, otherwise these inserts will fail.
89-138: Ensure the target database table schema includes the country column before deployment.The INSERT statement references a country column (line 99). Since this repository does not manage database migrations, verify that the target
public_cps_za_dlchangetable has been updated with the country column via your database migration tooling, otherwise inserts will fail with a column-not-found error.tests/writers/test_writer_postgres.py (1)
69-88: LGTM! Test assertions updated correctly.The parameter indices are correctly adjusted to account for the country field at position 6, and the test properly verifies the empty string default.
|
@copilot Why are changes made only for writer_postgres.py? There are other two writers. What were your decisions to implement it only somewhere? |
The EventBridge and Kafka writers serialize the entire message object as JSON, so they automatically include the
Only writer_postgres.py requires explicit column definitions in INSERT statements, which is why it needed updates to add the |
|
This PR is closed without a merge. It was created for learning purposes with agentic copilot mode. The solution have not satisfied the standard and will be created separately. |
Release Notes:
Adds an optional
countryfield to both run and EDLA (dlchange) schemas to support regional data tracking.Schema Changes
countryfield tojobs.items.properties(positioned aftercatalog_id)countryfield to rootproperties(positioned aftertimestamp_event)Both fields are optional string types with no default constraint in the schema definitions.
Database Writer Updates
Updated PostgreSQL insert operations to include the new column:
Default value of empty string when field is absent maintains backward compatibility with existing events.
Test Updates
Modified test assertions to validate the additional parameter position in SQL insert statements. Tests verify both presence of the field and correct default empty string behavior.
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.