Skip to content

Conversation

@ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Jun 20, 2023

Closes #130

Code Changes

  • add a new test that fails on checking for referenced fides keys
  • fix the code causing the issue

Steps to Confirm

  • list any manual steps taken to confirm the changes

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Found a bug where HTTPUrl parts were getting mistaken as FidesKeys. This is a quick patch for that bug, which also would've shown up in other places eventually.

@ThomasLaPiana ThomasLaPiana self-assigned this Jun 20, 2023
@ThomasLaPiana ThomasLaPiana requested a review from pattisdr June 20, 2023 08:23
@pattisdr
Copy link
Contributor

Reviewing now!

@ThomasLaPiana
Copy link
Contributor Author

The job got stuck waiting for a runner, but everything passed so I don't see a problem

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 tried pointing fideslang in fides to this latest commit and it's revealing other issues, although it looks like we're past that original organization issue:

https://github.com/ethyca/fides/actions/runs/5325046130/jobs/9645546378?pr=3572

Loading resource manifests from: demo_resources/
Taxonomy successfully created.
Evaluating the following policies:
- demo_privacy_policy
- data_sharing_policy
- primary_privacy_policy
----------
Checking for missing resources...
Missing resource keys: {'Processor - marketing co.', 'CAN', 'USA', 'GBR'}
>       assert (
            database_settings.sync_database_uri
            == "***fides-db:5432/database?sslmode=verify-full&sslrootcert=/etc/ssl/private/myca.crt"
        )
E       AssertionError: assert '***fides-db:5432/database?sslmode=verify-full&sslrootcert=%2Fetc%2Fssl%2Fprivate%2Fmyca.crt' == '***fides-db:5432/database?sslmode=verify-full&sslrootcert=/etc/ssl/private/myca.crt'
E         - ***fides-db:5432/database?sslmode=verify-full&sslrootcert=/etc/ssl/private/myca.crt
E         ?                                                                                             ^   ^   ^       ^
E         + ***fides-db:5432/database?sslmode=verify-full&sslrootcert=%2Fetc%2Fssl%2Fprivate%2Fmyca.crt
E         ?   

FAILED tests/ctl/cli/test_cli.py::TestEvaluate::test_evaluate_demo_resources_pass
FAILED tests/ctl/cli/test_cli.py::TestEvaluate::test_local_evaluate_demo_resources
FAILED tests/ctl/core/config/test_config.py::TestBuildingDatabaseValues::test_builds_with_params

@ThomasLaPiana
Copy link
Contributor Author

I tried pointing fideslang in fides to this latest commit and it's revealing other issues, although it looks like we're past that original organization issue:

https://github.com/ethyca/fides/actions/runs/5325046130/jobs/9645546378?pr=3572

Loading resource manifests from: demo_resources/
Taxonomy successfully created.
Evaluating the following policies:
- demo_privacy_policy
- data_sharing_policy
- primary_privacy_policy
----------
Checking for missing resources...
Missing resource keys: {'Processor - marketing co.', 'CAN', 'USA', 'GBR'}
>       assert (
            database_settings.sync_database_uri
            == "***fides-db:5432/database?sslmode=verify-full&sslrootcert=/etc/ssl/private/myca.crt"
        )
E       AssertionError: assert '***fides-db:5432/database?sslmode=verify-full&sslrootcert=%2Fetc%2Fssl%2Fprivate%2Fmyca.crt' == '***fides-db:5432/database?sslmode=verify-full&sslrootcert=/etc/ssl/private/myca.crt'
E         - ***fides-db:5432/database?sslmode=verify-full&sslrootcert=/etc/ssl/private/myca.crt
E         ?                                                                                             ^   ^   ^       ^
E         + ***fides-db:5432/database?sslmode=verify-full&sslrootcert=%2Fetc%2Fssl%2Fprivate%2Fmyca.crt
E         ?   

FAILED tests/ctl/cli/test_cli.py::TestEvaluate::test_evaluate_demo_resources_pass
FAILED tests/ctl/cli/test_cli.py::TestEvaluate::test_local_evaluate_demo_resources
FAILED tests/ctl/core/config/test_config.py::TestBuildingDatabaseValues::test_builds_with_params

Hmmm ok, then I'll need to add more tests and complicated logic for figuring out what is truly a FidesKey and what isn't 🤔 Thank you! I'll keep working on this!

@ThomasLaPiana ThomasLaPiana changed the title Fix organization relationship key finding Fix Key Finding Function Jun 21, 2023
@ThomasLaPiana ThomasLaPiana requested a review from pattisdr June 21, 2023 10:11
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.

looks good as far as I can tell, nice additional test coverage here

@pattisdr pattisdr merged commit e5ef634 into main Jun 21, 2023
@pattisdr pattisdr deleted the ThomasLaPiana-fix-organizations-key-finder branch June 21, 2023 20: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.

An update to the key finder function appears to have broken something when trying to upgrade in Fides

3 participants