Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

saas request overrides #986

Merged
merged 8 commits into from
Aug 1, 2022
Merged

saas request overrides #986

merged 8 commits into from
Aug 1, 2022

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Jul 28, 2022 •
edited
Loading

Purpose

Enables user (developer) to provide custom python extensions/overrides of saas request execution.

Changes

  • initial cut of saas request overrides framework
  • includes mailchimp override as an example and test case
  • also includes minor refactor of some of the saas request execution to enable smoother override

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section 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.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #815

@adamsachs adamsachs added enhancement New feature or request dev experience Enhancements to the overall DX run unsafe ci checks Triggers running of unsafe CI checks Needs doc review fidesops-plus Identifies work that is needed by the fidesops plus team labels Jul 28, 2022
@adamsachs adamsachs requested a review from galvana July 28, 2022 21:14
@adamsachs adamsachs self-assigned this Jul 28, 2022
@adamsachs adamsachs linked an issue Jul 28, 2022 that may be closed by this pull request
Python overrides for SaaS config endpoints #815
Closed
@adamsachs adamsachs marked this pull request as draft July 28, 2022 21:14
initial cut of saas request overrides. include mailchimp as an exampl…
bc0f741 
…e and test case. minor refactor of some of the saas request execution to enable smoother override
@adamsachs adamsachs force-pushed the 815-python-overrides-for-saas-config-endpoints branch from 611f1a6 to bc0f741 Compare July 28, 2022 22:05
@adamsachs adamsachs changed the title initial cut of saas request overrides saas request overrides Jul 28, 2022
galvana
galvana reviewed Jul 29, 2022
View reviewed changes
src/fidesops/service/saas_request/saas_request_override_factory.py

@classmethod
def register(
cls, name: str, request_types: List[SaaSRequestType]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is request_types a list for flexibility? I can't think of a use-case for this right now but it's better to have this just in case right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. the use case i could imagine was potentially around update/delete? probably wouldn't make sense at this point, but just thought i'd keep things flexible.

galvana
galvana reviewed Jul 29, 2022
View reviewed changes
src/fidesops/service/saas_request/override_implementations/mailchimp_request_overrides.py Outdated
"mailchimp_member_update", [SaaSRequestType.UPDATE]
)
def mailchimp_member_update(
update_param_values: List[Dict[str, Any]],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name update_param_values is a bit confusing since we have the concept of param_values. At first glance the for update_params in update_param_values part makes me think that we're iterating through each param_value. I think we need something that better describes this input as a list of param_values per row to update. Maybe param_values_list, or something along those lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, i agree the current name is confusing. i'm not sure param_values_list clears things up that much though. maybe just being very explicit and param_values_per_row ?

galvana
galvana reviewed Jul 29, 2022
View reviewed changes
src/fidesops/service/saas_request/saas_request_override_factory.py Outdated
raise InvalidSaaSRequestOverrideException(
"Provided SaaS request override function must return a List[Row]"
)
if len(sig.parameters) < 6:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts around validating the kwarg names and types? I imagine the signature should be consistent since we're calling the function with fixed inputs. What is some of the flexibility you're referring to in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i thought back and forth on this a bit, and am definitely not convinced on what is the best approach.

i guess i just don't really see the benefit of preventing an implementer from naming or type-annotating the args however they want in their implementation. whatever we do here in this validation, it's not going to really give them the IDE/static support to automatically generate their methods, or to inform them of what the arguments are, etc. - to get that understanding, they're going to need to refer to examples and/or documentation. so at that point, if they want to for whatever reason adjust the argument names from the example that they're referring to, or e.g. provide a more generic type annotation, i don't really see why we (the framework) should care at runtime.

i guess what i mean is, with the way things are set up now, you still really have to know what you're doing in order to be able to implement an override here. so it just seems unlikely that they'd end up in a scenario where having their function fail to register because of arg names or type annotations would really help them debug a problem. but maybe i'm not thinking creatively enough.

perhaps extending that argument further would suggest we shouldn't do any sort of validation. i thought these validations made sense because they're really what the framework cares about -- i.e. that it's going to be able to invoke the function successfully, and that it's going to get back from the function what it expects. besides that, we try to just treat the override function as a black box.

in any case, i'd definitely like to talk/think through this one more. i'll leave it unaddressed for the moment.

galvana
galvana reviewed Jul 29, 2022
View reviewed changes
src/fidesops/service/saas_request/saas_request_override_factory.py
with the given name.
"""

if isinstance(request_types, SaaSRequestType):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are lines 53-54 doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah it's just handling the case where the user passes in a single SaaSRequestType by converting it to a list behind the scenes for them. do you know of a more idiomatic way of doing that? is that a bad thing to do in your opinion? it seemed convenient and harmless to me, and also seemed like the sorta thing that people do in python, but i wouldn't take my word for it 😅

@adamsachs adamsachs marked this pull request as ready for review July 29, 2022 13:13
@adamsachs adamsachs added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Jul 29, 2022
@adamsachs
Copy link
Contributor Author

adamsachs commented Jul 29, 2022 •
edited
Loading

noting here that unsafe CI checks succeeded after 2 failed attempts, without any code changes, so there's something fishy going on with the unsafe CI test.

in this case, both failures were on the same assertion in the hubspot test erasure task -- the assertion that confirms the results of the access request that is accessing the erasure seed data. The same assertion failure occurred on this workflow run yesterday.

i split out #992

@adamsachs adamsachs mentioned this pull request Jul 29, 2022
Closed
galvana
galvana approved these changes Aug 1, 2022
View reviewed changes
Copy link
Collaborator

@galvana galvana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes and making the time to discuss some of the other points, this looks good to me!

@adamsachs adamsachs merged commit 0952c6c into main Aug 1, 2022
@adamsachs adamsachs deleted the 815-python-overrides-for-saas-config-endpoints branch August 1, 2022 21:20
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
@adamsachs
saas request overrides (#986)
4643668 
* initial cut of saas request overrides. include mailchimp as an example and test case. minor refactor of some of the saas request execution to enable smoother override

* fix rebase issue by moving saas override tests into ops subdir

* import path updates to resolve conflicts caused by rebase

* add session parameter into graph task calls to fix saas override integration tests caused by rebase

* update changelog

* tweaks to saas connector overrides and associated tests

* expose override factory register as module variable for clenaer decorator calls

Co-authored-by: Adam Sachs <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@galvana galvana galvana approved these changes

Assignees

@adamsachs adamsachs

Labels
dev experience Enhancements to the overall DX enhancement New feature or request fidesops-plus Identifies work that is needed by the fidesops plus team Needs doc review run unsafe ci checks Triggers running of unsafe CI checks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Python overrides for SaaS config endpoints
2 participants
@adamsachs @galvana