Eng 567 record integration version on exec logs by Vagoasdf · Pull Request #7650 · ethyca/fides

Vagoasdf · 2026-03-13T16:00:56Z

Ticket 567

Description Of Changes

Adding the integration version to the execution logs.
This would help during testing and monitoring to verify what version of the integration an instance is running

Code Changes

Migration that runs a saas version execution log update
Adding the saas version for the Retry and start Graph task´s log.
Adding the version as a column to the execution log on the view of the events

Steps to Confirm

Set up two saas integrations (any)
execute an access request
verify that both saas integrations are showing their version in the execution log

Pre-Merge Checklist

vercel · 2026-03-13T16:01:02Z

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Actions	Updated (UTC)
fides-plus-nightly	Ignored	Preview	Mar 19, 2026 2:56pm
fides-privacy-center	Ignored		Mar 19, 2026 2:56pm

Updating test for endpoints

greptile-apps · 2026-03-17T14:45:48Z

Greptile Summary

This PR records the SaaS integration config version (saas_version) on every ExecutionLog entry, making it easier to trace which version of an integration was running during a DSR execution. The change is well-scoped: a nullable Alembic migration, a model column, schema additions, a union-query update in the endpoint, and a new "Version" column in the admin UI event log table.

Key changes:

New nullable saas_version column added to the executionlog table via Alembic migration
GraphTask.__init__ extracts the version from the connector's raw saas_config dict at construction time and stamps it on every update_status call
log_start and log_retry logger messages also include the version when available
Admin UI EventLog table shows a versioned Tag when saas_version is present, or a - dash for non-SaaS connectors
The migration filename carries an xx_ placeholder prefix that is inconsistent with every other migration file in the repository (which use the hex revision hash as a prefix)
Two new model-layer tests call execution_log.delete(db) at the end, which is an unnecessary anti-pattern since the database is automatically cleared between test runs
One graph task test directly asserts on the private _saas_version attribute, which is fragile and redundant

Confidence Score: 4/5

Safe to merge after addressing the migration filename placeholder and the minor test clean-up issues.
The feature logic is correct and well-tested end-to-end. The only issues are cosmetic/convention-level: the migration file's xx_ prefix needs to be renamed, a couple of tests have unnecessary manual record deletions, and one test accesses a private attribute. None of these would cause a runtime failure.
The migration file xx_2026_03_11_0000_a1ca9ddf3c3c_add_saas_version_to_executionlog.py needs to be renamed to drop the xx_ placeholder prefix before merging.

Important Files Changed

Filename	Overview
src/fides/api/alembic/migrations/versions/xx_2026_03_11_0000_a1ca9ddf3c3c_add_saas_version_to_executionlog.py	Adds a nullable `saas_version` column to the `executionlog` table; migration logic is correct but the `xx_` filename prefix is a placeholder that should be renamed to match the repo's hex-hash naming convention.
src/fides/api/task/graph_task.py	Reads `saas_config["version"]` from the connector's raw dict at construction time and passes `_saas_version` into every `update_status` call and the `log_start`/`log_retry` logger messages. Logic is sound and handles None safely.
src/fides/api/v1/endpoints/privacy_request_endpoints.py	Adds `ExecutionLog.saas_version` to the execution log query and `null().label("saas_version")` to the audit log query so the union remains column-aligned. No issues found.
clients/admin-ui/src/features/privacy-requests/events-and-logs/EventLog.tsx	Adds a "Version" column and `<Td>` cell that renders the version as a tag or a dash placeholder, only shown when there are dataset entries and not in the request-finished view. No issues found.
tests/ops/models/privacy_request/test_execution_logs.py	Adds two new unit tests for `saas_version` persistence; both unnecessarily call `execution_log.delete(db)` at the end, which is an established anti-pattern in this codebase since the DB is cleared automatically between runs.
tests/ops/task/test_graph_task.py	Adds a `saas_graph_task` fixture and two integration tests; one test directly asserts on the private `_saas_version` attribute, which is fragile and redundant given the subsequent execution log assertion.

_{Last reviewed commit: 5ae13c1}

src/fides/api/alembic/migrations/versions/a1ca9ddf3c3c_add_saas_version_to_executionlog.py

greptile-apps · 2026-03-17T14:45:52Z

tests/ops/models/privacy_request/test_execution_logs.py

+def test_execution_log_saas_version_stored(db, execution_log_data):
+    execution_log_data["saas_version"] = "2.3.0"
+    execution_log = ExecutionLog.create(db, data=execution_log_data)
+
+    retrieved = db.query(ExecutionLog).filter_by(privacy_request_id="test_id").first()
+    assert retrieved is not None
+    assert retrieved.saas_version == "2.3.0"
+    execution_log.delete(db)


Manual record deletion in test — not needed

Both new tests call execution_log.delete(db) at the end. The database is automatically cleared between test runs, so this manual cleanup is unnecessary and goes against the testing conventions for this project. The cleanup should be removed from both new tests.

Suggested change

def test_execution_log_saas_version_stored(db, execution_log_data):

execution_log_data["saas_version"] = "2.3.0"

execution_log = ExecutionLog.create(db, data=execution_log_data)

retrieved = db.query(ExecutionLog).filter_by(privacy_request_id="test_id").first()

assert retrieved is not None

assert retrieved.saas_version == "2.3.0"

execution_log.delete(db)

def test_execution_log_saas_version_stored(db, execution_log_data):

execution_log_data["saas_version"] = "2.3.0"

ExecutionLog.create(db, data=execution_log_data)

retrieved = db.query(ExecutionLog).filter_by(privacy_request_id="test_id").first()

assert retrieved is not None

assert retrieved.saas_version == "2.3.0"

The same applies to test_execution_log_saas_version_null_for_non_saas at line 122.

Rule Used: Do not manually delete database records in test fi... (source)

Learnt From
ethyca/fides#6688

_{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-apps · 2026-03-17T14:45:54Z

tests/ops/task/test_graph_task.py


        assert execution_log.status == ExecutionLogStatus.complete

+    def test_saas_version_null_for_non_saas_connector(
+        self, graph_task, db, privacy_request
+    ):
+        """Non-SaaS connectors (postgres in this fixture) should produce null saas_version."""
+        graph_task.log_start(action_type=ActionType.access)
+
+        execution_log = (
+            db.query(ExecutionLog)
+            .filter(
+                ExecutionLog.privacy_request_id == privacy_request.id,
+                ExecutionLog.collection_name == "b",
+                ExecutionLog.dataset_name == "a",
+            )
+            .first()
+        )
+        assert execution_log is not None
+        assert execution_log.saas_version is None
+
+    def test_saas_version_populated_for_saas_connector(
+        self, saas_graph_task, saas_example_connection_config, privacy_request, db
+    ):


Accessing private instance attribute in test

saas_graph_task._saas_version directly accesses a private implementation detail of GraphTask. This assertion is mostly redundant given that the test already verifies the saas_version is correctly propagated to the execution log record (the real observable behavior). The assertion on the private field is fragile — if the internal attribute is renamed or restructured it breaks without any functional regression.

Consider removing this assertion and relying solely on the execution log verification that follows, or exposing a public property if the attribute needs to be observable from outside the class.

Suggested change

assert execution_log.status == ExecutionLogStatus.complete

def test_saas_version_null_for_non_saas_connector(

self, graph_task, db, privacy_request

):

"""Non-SaaS connectors (postgres in this fixture) should produce null saas_version."""

graph_task.log_start(action_type=ActionType.access)

execution_log = (

db.query(ExecutionLog)

.filter(

ExecutionLog.privacy_request_id == privacy_request.id,

ExecutionLog.collection_name == "b",

ExecutionLog.dataset_name == "a",

)

.first()

)

assert execution_log is not None

assert execution_log.saas_version is None

def test_saas_version_populated_for_saas_connector(

self, saas_graph_task, saas_example_connection_config, privacy_request, db

):

def test_saas_version_populated_for_saas_connector(

self, saas_graph_task, saas_example_connection_config, privacy_request, db

):

"""SaaS connectors should stamp saas_version on every execution log entry."""

expected_version = saas_example_connection_config.saas_config["version"]

saas_graph_task.log_start(action_type=ActionType.access)

_{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!}

- Use conditional logger.info calls instead of mixing f-strings with loguru placeholders in log_start/log_retry - Add comment explaining _saas_version is snapshotted at construction time - Fix missing blank line between test functions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

johnewart

The backend stuff looks good - the frontend bits seem sane but I will let someone else confirm that 👍🏼

Vagoasdf added 7 commits March 12, 2026 11:24

Adding base executionlog saas version

cd09324

adding execution log saas version model

85ab8eb

adding saas_version on the logs

396c00a

adding test for the execution log

a39d035

Adding Test for the graph task

db4557d

adding logging on the graph task

b5d0ea7

adding saas_version to the audit query

9899f9d

Vagoasdf added 13 commits March 13, 2026 13:10

Fixing Revision name and settings

7baae34

Caching Saas version

1053fb4

Updating tests

4600544

Event logs UI adding version

98a5629

Updating dabase dataset.

a9ca916

Updating test for endpoints

using terniaries and avoiding long expressions

be47458

Removing unnecesary policy

f536355

Adding version in its own column

e5a4515

Adding changelog

caee311

Merge branch 'main' into ENG-567_record-integration-version-on-exec-logs

519350f

Updating revision

47253a2

running ruff

db3babc

Adding saas_versions to endpoint tests

39ecea6

Vagoasdf marked this pull request as ready for review March 17, 2026 14:42

Vagoasdf requested review from a team as code owners March 17, 2026 14:42

Vagoasdf requested review from johnewart and removed request for a team March 17, 2026 14:42

Merge branch 'main' into ENG-567_record-integration-version-on-exec-logs

5ae13c1

Vagoasdf requested review from lucanovera and removed request for a team March 17, 2026 14:42

greptile-apps bot reviewed Mar 17, 2026

View reviewed changes

Vagoasdf and others added 6 commits March 17, 2026 12:44

Renaming migration

ca3149d

Updating down revision

efe6d6b

Fixing up greptile comments on tests

d564b9b

Renaming migration

836d76a

Merge branch 'main' into ENG-567_record-integration-version-on-exec-logs

a1552d4

vercel bot deployed to Preview – fides-plus-nightly March 19, 2026 14:50 View deployment

Updating the migration down revision again

0e1066c

johnewart approved these changes Mar 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eng 567 record integration version on exec logs#7650

Eng 567 record integration version on exec logs#7650
Vagoasdf wants to merge 28 commits intomainfrom
ENG-567_record-integration-version-on-exec-logs

Vagoasdf commented Mar 13, 2026 •

edited

Loading

Uh oh!

vercel bot commented Mar 13, 2026 •

edited

Loading

Uh oh!

greptile-apps bot commented Mar 17, 2026

Uh oh!

Uh oh!

greptile-apps bot Mar 17, 2026

Uh oh!

greptile-apps bot Mar 17, 2026

Uh oh!

johnewart left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Vagoasdf commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

Uh oh!

vercel bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps bot commented Mar 17, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

Uh oh!

greptile-apps bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

johnewart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Vagoasdf commented Mar 13, 2026 •

edited

Loading

vercel bot commented Mar 13, 2026 •

edited

Loading