Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid error if access results key is not found for erasures #2045

Merged
merged 2 commits into from
Dec 15, 2022

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Dec 14, 2022

Closes #2048

Code Changes

  • default to an empty list [] if, in the beginning of erasure request execution, no access result cache entry is found for the given collection

Steps to Confirm

  • See issue description steps to reproduce, and ensure that requests no longer error in this scenario, but instead go to COMPLETE
  • Also can confirm that the Event Details for the step associated with the disabled connector show all of its events with skipped status

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • 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)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Passing in the empty list is effectively a no-op here, just a way to prevent an error. This is because the list doesn't actually get used, as the erasure request execution will not proceed because the connector is disabled.

@adamsachs adamsachs force-pushed the explore-disabled-connectors-bugs branch from a2aaa72 to f13b259 Compare December 15, 2022 14:16
Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

nice catch @adamsachs

src/fides/api/ops/task/graph_task.py Outdated Show resolved Hide resolved
in cases where connector is disabled, there won't be an entry
in the acces results for that connector's key.
instead of throwing a key error, we instead just pass in an empty list
to the erasure execution, which will not actually execute anyway.
@adamsachs adamsachs force-pushed the explore-disabled-connectors-bugs branch from f13b259 to 3a6dd3d Compare December 15, 2022 14:51
@adamsachs adamsachs self-assigned this Dec 15, 2022
@adamsachs adamsachs marked this pull request as ready for review December 15, 2022 15:32
@adamsachs
Copy link
Contributor Author

adamsachs commented Dec 15, 2022

integration test failure seems like a blip (it's only on one python env) and unrelated to changes, so i'll go ahead and merge if that looks right to you @pattisdr ?

@pattisdr
Copy link
Contributor

I agree @adamsachs this is likely unrelated and resolved by re-running!

@adamsachs
Copy link
Contributor Author

CI is being stubborn with the integration tests. the tests are passing locally and there's no connection between the failure reported here and the changed code. Merging rather than fighting this more, i think we're good!

@adamsachs adamsachs merged commit e77d6dc into main Dec 15, 2022
@adamsachs adamsachs deleted the explore-disabled-connectors-bugs branch December 15, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabled connectors cause erasure requests to error
2 participants