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

Integrate generating a dataset from a database connection #889

Merged
merged 4 commits into from
Jul 12, 2022

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Jul 11, 2022

Closes #829

Code Changes

  • Runs openapi generate again to update types for latest backend changes to /generate endpoint (worked beautifully 💯 )
  • Add call to /generate endpoint from dataset.slice.ts
  • After a generate call, use the result to make a call to dataset POST

Steps to Confirm

  • Try to create a dataset by connecting to a database. I used a redshift db Eduardo had set up (message me for credentials!) or can also spin up a local database, i.e. the flaskr one in fidesdemo. Note if you do that, if you are on a Mac you'll want to use this postgres string: postgresql://postgres:[email protected]:6432/flaskr (because of docker networking stuff)

Pre-Merge Checklist

Description Of Changes

One thing I'm not sure how to approach is that we need an organization key to use the generate endpoint. I hard coded default_organization for now, but I'm not sure how we plan to go about getting that in the future (potentially related to config UI work? is it possible we can always have an organization key around, similar to how we'd have a user around?)

Success state

Screen.Recording.2022-07-11.at.5.21.21.PM.mov

Errors

Untitled.mov

Error if the string just doesn't work (not sure if the backend can give a better error than just a 500, or if there's any validation we should do on the frontend before sending it over)

image

@allisonking allisonking marked this pull request as ready for review July 11, 2022 21:39
@allisonking allisonking requested a review from a team July 11, 2022 21:39
@ThomasLaPiana
Copy link
Contributor

@allisonking I think we'll tie user to organization eventually? so that should hopefully fix that 🤔 i might be wrong on that though tbh, I don't think we've discussed yet exactly where the user will set (extra or intra system)

Copy link
Contributor

@ThomasLaPiana ThomasLaPiana left a comment

Choose a reason for hiding this comment

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

LGTM!

@ThomasLaPiana
Copy link
Contributor

@allisonking i tested locally with the following string and all worked as expected: postgresql://postgres:fidesctl@fidesctl-db:5432/fidesctl_test

However the error message is super unclear if the URL is wrong, and it doesn't say anything like "malformed URL" vs "properly formed but unreachable". I know a lot of that will need to get done on the backend though so can we open another ticket to add more robust backend error messaging to that endpoint?

I'm thinking something along the lines of the error messaging we do for the webserver itself, where we can clearly tell the user why it didn't work as opposed to just "it didn't work"

@allisonking allisonking merged commit 7f3fcad into main Jul 12, 2022
@allisonking allisonking deleted the aking-829-db-dataset branch July 12, 2022 14:06
allisonking added a commit that referenced this pull request Jul 28, 2022
* Update types for new db generate functionality

* Integrate adding a dataset via a database

* Clean up

* Update changelog
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.

Integrate the Dataset DB connection endpoint to the UI
2 participants