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

Fidesops 78 mssql support #151

Merged
merged 16 commits into from
Jan 10, 2022
Merged

Fidesops 78 mssql support #151

merged 16 commits into from
Jan 10, 2022

Conversation

eastandwestwind
Copy link
Contributor

@eastandwestwind eastandwestwind commented Jan 6, 2022

Purpose

Adds SQL Server support

Changes

  • Adds connection support for mssql
  • Adds connection integration tests for mssql
  • To manually test, in Postman PUT {{host}}/connection/{{mssql_key}}/secret, use the following url: "mssql+pyodbc://sa:Mssql_pw1@mssql_example:1433/mssql_example?driver=ODBC+Driver+17+for+SQL+Server", alternatively you can break each part out into separate components, refer to connection_secrets_mssql.py.

Checklist

  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram
  • Good unit test/integration test coverage

Ticket

Fixes #78

@pattisdr pattisdr self-assigned this Jan 7, 2022
@pattisdr pattisdr added the run unsafe ci checks Triggers running of unsafe CI checks label Jan 7, 2022
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 work @eastandwestwind - the main additional piece I'd like to see is to try to get the mssql_example database created and populated when we run make integration-env, not just creating it in the integration tests

src/fidesops/models/connectionconfig.py Show resolved Hide resolved
pytest.ini Outdated Show resolved Hide resolved
tests/integration_tests/test_integration_mssql_example.py Outdated Show resolved Hide resolved
tests/integration_tests/test_integration_mssql_example.py Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@eastandwestwind
Copy link
Contributor Author

Ok, @pattisdr , over to you again! I cleaned up the PR based on your suggestions, and was able to get logs to print for make integration-env using your suggestion: @docker-compose -f docker-compose.yml -f docker-compose.integration-test.yml logs -f -t.

I also reused the @docker exec -it fidesops python tests/integration_tests/mssql_setup.py step for spinning up the test db for integration tests.

@pattisdr
Copy link
Contributor

great will take a look!

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 @eastandwestwind 🏆 - i really liked your solution to bring up the docker containers in detached mode, then exec into the fidesops container to run the python mssql initialization script to get around the M1 + MSSQL issues when we're trying to create a test mssql_example database on make integration-env.

just one quick point and I think this is good to go

engine.execute(sqlalchemy.sql.text(query))


mssql_setup()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might want to only call this if we're running this file directly (which will set the __name__variable as __main__) and not call this when we import it:

if __name__ == "__main__":
    mssql_setup()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohh I see, good catch!

@pattisdr pattisdr merged commit 0157344 into main Jan 10, 2022
@pattisdr pattisdr deleted the fidesops_78_mssql_support branch January 10, 2022 23:21
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
Co-authored-by: Dawn Pattison <[email protected]>
Co-authored-by: catherinesmith <[email protected]>
Co-authored-by: Catherine Smith <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
run unsafe ci checks Triggers running of unsafe CI checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectionConfig for Microsoft SqlServer
2 participants