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

Update fides deploy to use a new database.load_samples setting to initialize sample Systems, Datasets, and Connections for testing #3102

Merged
merged 40 commits into from
Apr 24, 2023

Conversation

NevilleS
Copy link
Contributor

@NevilleS NevilleS commented Apr 19, 2023

Code Changes

  • Move cookie_house sample app to clients/cookie_house instead of buried in the sample_project/ dir
  • Fix Cypress import errors when running locally via nox -s cypress_tests
  • Add new FIDES__DATABASE__LOAD_SAMPLES config setting to automatically load sample data on startup
  • Add load_sample_resources_from_project that parses the resource YMLs (systems, datasets, etc), instead of relying on fides push
  • Add load_sample_resources_from_project that parses a (new) connections YML to configure various database & SaaS connectors , instead of relying on load_examples.py
  • Remove the seed_example_data step from fides deploy, and use the "load samples" config setting instead 👍
  • Cleanup startup logging to be less verbose
  • Tests for all the above

Steps to Confirm

  • Run nox - s "fides_env(test)" and confirm that sample systems, datasets, and Postgres & Mongo connectors work
  • Edit .env to include Mailchimp, Stripe, or Hubspot secrets and confirm those connectors are created
  • Run nox -s e2e_test to confirm the Cypress smoke tests still work as expected
  • Edit sample_project/sample_connectors.yml to add/remove connectors and confirm they're included in the test env

Pre-Merge Checklist

Description Of Changes

While continuing to overhaul the reusability of our test environment, I've found that it's generally unreliable to use Docker commands to run scripts to populate sample resources, connections, etc. It's a slightly long story, but overall the seed_example_data function in the fides deploy command is tightly coupled to the fides image having all the source code files available, and that's not reliable when we try to reuse this setup with different images (e.g. fidesplus, or custom fides images, etc.)

This is a fairly large refactor of the "sample data" part of the fides deploy script that removes the need to run scripts to make API requests against the server to setup sample data, and instead adds a new config setting, FIDES__DATABASE__LOAD_SAMPLES which will automatically load sample data into the database at startup time. In doing this, I also converted the "sample connectors" part of the load_examples.py script into a sample_connections.yml file that defines the connectors that should be supported by the fides deploy command. This makes it very easy to add/remove connectors by editing that YML file and providing the necessary secrets as ENV vars.

The messiest part here is the seed.py logic that imports & reuses existing API endpoints, but calls them directly instead of as part of HTTP request handling...

@cypress
Copy link

cypress bot commented Apr 19, 2023

Passing run #1498 ↗︎

0 3 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 65afd2e into d39da31...
Project: fides Commit: 0148997556 ℹ️
Status: Passed Duration: 00:54 💡
Started: Apr 21, 2023 4:59 PM Ended: Apr 21, 2023 5:00 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@NevilleS
Copy link
Contributor Author

Hmm, the fides container isn't coming up healthy in some of the test suites here. I'll have to troubleshoot that a bit; that shouldn't have been affected, but... 🤔

Copy link
Contributor Author

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

Notes for reviewers!

example.env Show resolved Hide resolved
noxfiles/docker_nox.py Outdated Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
src/fides/api/ctl/database/samples.py Outdated Show resolved Hide resolved
tests/ctl/api/test_seed.py Outdated Show resolved Hide resolved
tests/ctl/api/test_seed.py Show resolved Hide resolved
tests/ctl/api/test_seed.py Show resolved Hide resolved
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.

I like this @NevilleS good code comments and great tests to protect your code. It's a little duplicative in places but you've motivated it well. Thanks for the self-review.

example.env Outdated Show resolved Hide resolved
src/fides/api/main.py Outdated Show resolved Hide resolved
src/fides/api/ctl/database/seed.py Outdated Show resolved Hide resolved
src/fides/api/ctl/database/seed.py Show resolved Hide resolved
src/fides/api/ctl/database/samples.py Outdated Show resolved Hide resolved
@NevilleS
Copy link
Contributor Author

oof - I think I know why some of the tests are failing. It looks like it's probably specific to the importlib.resources.files() API I'm using, which requires Python 3.9 due to some bugs in 3.8 (I wasn't paying close enough attention there...!)

I think I'll need to figure out how to use "futures" or use the importlib_resources lib that backports to 3.8, etc.

sigh

@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Patch coverage: 81.57% and project coverage change: -0.04 ⚠️

Comparison is base (d39da31) 87.56% compared to head (65afd2e) 87.52%.

❗ Current head 65afd2e differs from pull request most recent head 8729849. Consider uploading reports for the commit 8729849 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3102      +/-   ##
==========================================
- Coverage   87.56%   87.52%   -0.04%     
==========================================
  Files         308      309       +1     
  Lines       17742    17839      +97     
  Branches     2288     2310      +22     
==========================================
+ Hits        15535    15613      +78     
- Misses       1792     1802      +10     
- Partials      415      424       +9     
Impacted Files Coverage Δ
src/fides/cli/commands/util.py 81.03% <ø> (ø)
src/fides/core/deploy.py 38.70% <ø> (+0.12%) ⬆️
src/fides/api/ctl/database/crud.py 74.61% <30.00%> (+0.76%) ⬆️
src/fides/api/ctl/database/seed.py 91.75% <82.95%> (-8.25%) ⬇️
src/fides/api/ctl/database/database.py 85.91% <83.33%> (-1.05%) ⬇️
src/fides/api/ctl/database/samples.py 86.84% <86.84%> (ø)
src/fides/api/main.py 79.39% <100.00%> (+0.38%) ⬆️
src/fides/cli/options.py 95.60% <100.00%> (ø)
src/fides/core/config/database_settings.py 100.00% <100.00%> (ø)
src/fides/core/config/helpers.py 100.00% <100.00%> (ø)

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@NevilleS
Copy link
Contributor Author

OK, I fixed the problem that was causing failures in Python 3.8 (thanks, CI!) and finished tidying things up.

At this point, it's ready to merge, but it has a major caveat- it removes the ability to automatically configure the S3 & Email features. I don't want to break that for testers, so I need to just go back and rethink this.

@pattisdr
Copy link
Contributor

Looking..

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.

great @NevilleS 🏆

@NevilleS
Copy link
Contributor Author

Added one more tweak to check the installed Python version is one we support, but otherwise this is good to go.

I'm going to wait to merge this until I get some more folks to test it out though, especially in it's fidesplus variant.

@NevilleS NevilleS removed the do not merge Please don't merge yet, bad things will happen if you do label Apr 24, 2023
@NevilleS NevilleS merged commit a7a4b10 into main Apr 24, 2023
@NevilleS NevilleS deleted the ns-update-fides-deploy branch April 24, 2023 11:04
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.

3 participants