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

handle non-existent firebase users #2439

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Jan 31, 2023

Closes #2425

Code Changes

  • catch expected exception and log warning rather than erroring out

Steps to Confirm

  • automated tests should cover us pretty well here...

Pre-Merge Checklist

Description Of Changes

Write some things here about the changes and any potential caveats

@adamsachs adamsachs linked an issue Jan 31, 2023 that may be closed by this pull request
@adamsachs adamsachs added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Jan 31, 2023
@adamsachs adamsachs marked this pull request as ready for review January 31, 2023 02:05
@adamsachs adamsachs self-assigned this Jan 31, 2023
@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Base: 88.42% // Head: 88.45% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (d031d63) compared to base (1b9a2c0).
Patch coverage: 23.07% of modified lines in pull request are covered.

❗ Current head d031d63 differs from pull request most recent head 9246d4c. Consider uploading reports for the commit 9246d4c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2439      +/-   ##
==========================================
+ Coverage   88.42%   88.45%   +0.02%     
==========================================
  Files         327      327              
  Lines       15892    15957      +65     
  Branches     4416     4431      +15     
==========================================
+ Hits        14053    14115      +62     
- Misses       1685     1688       +3     
  Partials      154      154              
Impacted Files Coverage Δ
...implementations/firebase_auth_request_overrides.py 28.43% <23.07%> (-0.30%) ⬇️
src/fides/core/utils.py 82.10% <0.00%> (ø)
src/fides/core/api_helpers.py 100.00% <0.00%> (ø)
src/fides/core/annotate_dataset.py 35.61% <0.00%> (ø)
src/fides/core/config/user_settings.py 100.00% <0.00%> (ø)
src/fides/api/ops/api/v1/scope_registry.py 100.00% <0.00%> (ø)
src/fides/core/config/security_settings.py 100.00% <0.00%> (ø)
src/fides/api/ctl/routes/crud.py 87.32% <0.00%> (+0.36%) ⬆️
src/fides/api/ctl/routes/admin.py 96.15% <0.00%> (+0.50%) ⬆️
src/fides/cli/__init__.py 92.85% <0.00%> (+0.54%) ⬆️
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adamsachs
Copy link
Contributor Author

failing tests seem unrelated to changes:

  • ctl-external tests (we don't touch anything ctl related here)
  • saas-integration tests are other flaky tests, nothing firebase related. our changes here are strictly scoped to firebase

codecov failure seems to be a remnant of previous runs that are not getting updated because of above unrelated failures - at least that is my and @sanders41 hypothesis. running codecov locally shows that the changed lines here do have coverage:

root@c5dcc8d4499c:/fides# pytest tests/ops/integration_tests/saas/request_override/test_firebase_auth_task.py --cov-report=term-missing
...

src/fides/api/ops/service/saas_request/override_implementations/firebase_auth_request_overrides.py               102      2     30      4    95%   63, 85->83, 88->99, 181

Copy link
Contributor

@sanders41 sanders41 left a comment

Choose a reason for hiding this comment

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

@adamsachs and I discussed the missing coverage lines. We can show that these lines are being covered when running the tests locally, and believe that the other non-related flaky tests failures are preventing the coverage report from being update showing the lines are covered.

@adamsachs adamsachs merged commit d00b2d6 into main Feb 2, 2023
@adamsachs adamsachs deleted the 2425-dsr-execution-failing-when-a-user-is-not-found-in-firebase branch February 2, 2023 15:14
@pattisdr
Copy link
Contributor

pattisdr commented Feb 2, 2023

@adamsachs
Copy link
Contributor Author

adamsachs commented Feb 2, 2023

you're right @pattisdr :( we missed that one, in a bit of a rush. thank you for the find 🙏

i'd mistakenly removed the integration_saas marker from that test, so it was running during the unit test run, when it doesn't have the context/secrets it needs. i've re-added the marker as part of this pending PR - 4bc8f43 - just to reduce paperwork getting a patch in to main ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DSR execution failing when a user is not found in Firebase
3 participants