Utility to update saas configs and datasets on system startup #1307
Conversation
adamsachs
force-pushed
the
1098-update-saas-configs-and-datasets-on-system-startup
branch
from
ca2bcb8 to
f0ee6cc
Compare
|
a large amount of the added code here is from the test cases, apologies for that. i couldn't think of a way to make the test cases more concise while keeping them robust - they required a fair amount of rather cumbersome mocking. i'm open to any suggestions on how to make those tests more concise and/or readable! |
Utility is invoked on server bootstrap. Updates made to the connector template registry over time are meant to be automatically applied to existing SaaS connector instance configurations that are already present in the DB.
adamsachs
force-pushed
the
1098-update-saas-configs-and-datasets-on-system-startup
branch
from
90a25ae to
eabe839
Compare
There was a problem hiding this comment.
nice refactoring work here @adamsachs to re-use existing code.
my primary concern here is the (likely, in my opinion) scenario when a version upgrade fails.
src/fidesops/ops/service/connectors/saas/connector_registry_service.py
Outdated
Show resolved
Hide resolved
src/fidesops/ops/service/connectors/saas/connector_registry_service.py
Outdated
Show resolved
Hide resolved
Instead of deleting existing configs, update in place. Simplify logic to not validate secrets unnecessarily. Revert intsantation endpoint to existing implementation as it no longer shares functionality with update logic.
Revert instantantiation endpoint test back to its original functionality
|
@pattisdr would you be able to take a look at my latest changes (https://github.com/ethyca/fidesops/pull/1307/files/eabe839e22b14378a46357b2eca5ac55e27ceb62..6ec9f492f6ed66b307e2804181bf49d5f66b8f3e) to see if they address your core concerns? specifically, i refactored the update logic to no longer delete configs from the db, but instead update in place. while making these changes, i realized we should be able to do a much more "targeted" replace -- i.e., just replace the similarly, i don't think we need to actually validate secrets on this update path - in fact, i don't think we want to, per your concern about the common error case of adding a secret to the template. we should aim to keep any existing configured secrets in tact, and we should add any default values provided by the new template if applicable (which the proposed changes do). validating the secrets in this use case is nonsensical IMO, since the user isn't actually inputting any new secrets. of course, this comment still applies, and we should make a new ticket to alert/warn the user about new secrets added to a template. effectively, i think the new changes here both make it less likely for us to error on template updates, and also make things safer if we happen to error. i think the code also ends up a bit cleaner. in the meantime, i'll look into cleaning up some of the core tests, which i still haven't addressed. i just wanted to get this new approach up for your review sooner rather than later. |
There was a problem hiding this comment.
nice @adamsachs I like this approach much better, doing more targeted updates to the connectionconfig.saas_config and updating the existing datasetconfig instead of a delete/create.
src/fidesops/ops/service/connectors/saas/connector_registry_service.py
Outdated
Show resolved
Hide resolved
…' of https://github.com/ethyca/fidesops into 1098-update-saas-configs-and-datasets-on-system-startup
Include some variable renaming to try to clarify test cases
There was a problem hiding this comment.
Nice work here @adamsachs.
Discussed separately, there's some followups needed to surface to the user if they need to update their secrets, or some portion of the automatic update failed so the user knows they need to adjust on their end, without it just being buried in the logs. Followup ticket here #1325
I'll leave it to your team to merge.
adamsachs
deleted the
1098-update-saas-configs-and-datasets-on-system-startup
branch
* Add utility to update SaaS connector instances based on template updates Utility is invoked on server bootstrap. Updates made to the connector template registry over time are meant to be automatically applied to existing SaaS connector instance configurations that are already present in the DB. * Refactor SaaS config instantation to use shared helpers * Update changelog * Improve SaaS config update logic to safely update configs Instead of deleting existing configs, update in place. Simplify logic to not validate secrets unnecessarily. Revert intsantation endpoint to existing implementation as it no longer shares functionality with update logic. * Improve db session management to use and close shared session * Update fixture to not rely on API endpoints Revert instantantiation endpoint test back to its original functionality * Remove new get_config classmethod as it is no longer needed * Adjust dataset creation function name for clarity * Fix up outdated mock function reference * Add test coverage for removal or addition of a connector param Include some variable renaming to try to clarify test cases Co-authored-by: Adam Sachs <[email protected]>
Purpose
Utility run on server bootstrap to update SaaS config instances in the DB based on updates to SaaS config or dataset template updates in the template registry
Changes
Checklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1098