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

Add UI for configuring integrations for data detection/discovery #4922

Merged

Conversation

jpople
Copy link
Contributor

@jpople jpople commented May 24, 2024

Closes PROD-2050, PROD-2055, PROD-2056, PROD-2069, PROD-2070

Description Of Changes

Adds UI for configuring integrations for data detection and discovery.

Screenshot 2024-05-24 at 09 35 06
Screenshot 2024-05-24 at 09 35 56
Screenshot 2024-05-24 at 09 39 24
Screenshot 2024-05-24 at 09 39 50

Code Changes

  • Add new list view, detail view, and form to create (for now only BigQuery) integrations behind detection & discovery beta flag
  • Changes to system information form to link to new integrations page when available and have fixed header

Steps to Confirm

With "Data detection & discovery" beta flag enabled:

  • Go to /integrations
  • If you have an existing BigQuery integration set up, should see that in list, otherwise should see empty state
  • Click "add integration" and step through form, using keyfile credentials JSON
  • Integration should show in list view
  • Clicking "Manage" should show integration detail view with overview and instructions (only "Connection" tab for now)

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

Lucano Vera and others added 30 commits May 13, 2024 18:25
…scovery' of https://github.com/ethyca/fides into PROD-2050-Configuring-integrations-for-DSRs-and-Data-Discovery
@jpople jpople marked this pull request as ready for review May 29, 2024 15:17
cy.getByTestId(testIdMulti)
.find(".custom-select__menu-list")
.contains("Eevee")
.click();
Copy link
Contributor Author

@jpople jpople May 29, 2024

Choose a reason for hiding this comment

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

I honestly have no idea why this PR caused this test to break, but I fixed it! ¯\_(ツ)_/¯


const isLoading = secretsIsLoading || patchIsLoading || systemPatchIsLoading;

const initialValues: FormValues = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that keyfile credentials aren't included in the initial values even if the user is editing-- the API response just returns "**********" and not populating the form with that is consistent with behavior in the other connection forms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. Can we a small comment in the code indicating that?

Copy link
Contributor

@lucanovera lucanovera left a comment

Choose a reason for hiding this comment

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

Overall looks very good for this iteration. Just have a couple comments to improve the code.

<>
<IntegrationBox
integration={BQ_PLACEHOLDER}
button={
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to pass the button to the IntegrationBox component? Is it going to look very different depending on the Integration?
If not, I think it would be better if the button was part of the IntegrationBox component. The IntegrationBox component could then have a "onAccept" or "onConfigure" prop so we can do the "setStep(1)" here.


const isLoading = secretsIsLoading || patchIsLoading || systemPatchIsLoading;

const initialValues: FormValues = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. Can we a small comment in the code indicating that?

</Box>
),
},
// {
Copy link
Contributor

Choose a reason for hiding this comment

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

some commented code here we could delete


const { onOpen, isOpen, onClose } = useDisclosure();

// const onTabChange = () => {};
Copy link
Contributor

Choose a reason for hiding this comment

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

commented code we can delete

Copy link
Contributor

@lucanovera lucanovera left a comment

Choose a reason for hiding this comment

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

Just saw the changes based on the review. Approve!

@jpople jpople merged commit 8309e83 into main May 30, 2024
13 checks passed
@jpople jpople deleted the PROD-2050-Configuring-integrations-for-DSRs-and-Data-Discovery branch May 30, 2024 14:58
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