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

Gong access and erasure #4461

Merged
merged 16 commits into from
Dec 5, 2023
Merged

Gong access and erasure #4461

merged 16 commits into from
Dec 5, 2023

Conversation

MarcGEthyca
Copy link
Contributor

@MarcGEthyca MarcGEthyca commented Nov 27, 2023

Closes #CON-108

Description Of Changes

Adding support for a new connector for Gong

Code Changes

  • Added connector related files for Gong connector

Steps to Confirm

  • Ensure you try adding this new connector in Fides

Pre-Merge Checklist

@galvana galvana changed the title Hopefully clean PR Gong access and erasure Nov 27, 2023
@galvana galvana marked this pull request as ready for review November 27, 2023 20:00
connector_params:
- name: domain
default: api.gong.io
- name: basic token
Copy link
Contributor

Choose a reason for hiding this comment

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

We should prompt the user for access_key and access_key_secret since that's what we use for basic authentication below. Only access_key_secret should be flagged with sensitive: True

Copy link
Contributor Author

@MarcGEthyca MarcGEthyca Nov 28, 2023

Choose a reason for hiding this comment

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

Made these changes.


connector_params:
- name: domain
default: api.gong.io
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here, it should be default_value and make sure to tab it in to line up with name

- name: domain
default: api.gong.io
- name: basic token
description: Check Gong's documentation for how to create this value https://us-66708.app.gong.io/settings/api/documentation#overview
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just use https://app.gong.io/settings/api/documentation#overview here, try not to put our Ethyca-specific URLs in the descriptions if possible

path: /v2/data-privacy/erase-data-for-email-address
body: |
{
"emailAddress": <delemail>
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure to include the param_values here and replace <delemail> with "<email>"

param_values:
    - name: email
      identity: email

def gong_secrets(saas_config) -> Dict[str, Any]:
return {
"domain": pydash.get(saas_config, "gong.domain") or secrets["domain"],
# add the rest of your secrets here
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this comment after you add in your secrets


@pytest.fixture
def gong_external_references() -> Dict[str, Any]:
return {}
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't use any of these placeholder fixtures (they just return {}) then you can delete the entire fixture and remove them as a parameter from the gong_runner function below

)
assert access_results == {"gong_instance:user": 1}

async def test_strict_erasure_request(
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this since we're going to use deletes (non-strict erasure)

Copy link
Contributor

Choose a reason for hiding this comment

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

This icon needs to be 32 x 32 units in Figma and have 2 units of padding all around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think I fixed it.
Here is what I am not clear on, do we mean that the icon frame has to be 32x32 (and can not be smaller?). Then I want to have the picture (without any fill) within the frame with 2 spaces of padding all around? I had been making them 28x28 I thought it was 4 spaces of padding all around.

Copy link

vercel bot commented Nov 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Dec 5, 2023 5:58pm

Copy link

cypress bot commented Nov 27, 2023

Passing run #5492 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge c9f7a5c into 5dab33f...
Project: fides Commit: fee5743692 ℹ️
Status: Passed Duration: 00:37 💡
Started: Dec 5, 2023 6:10 PM Ended: Dec 5, 2023 6:11 PM

Review all test suite changes for PR #4461 ↗︎

Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c500ab) 87.10% compared to head (19da622) 87.10%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4461   +/-   ##
=======================================
  Coverage   87.10%   87.10%           
=======================================
  Files         329      329           
  Lines       20386    20386           
  Branches     2627     2627           
=======================================
  Hits        17758    17758           
  Misses       2163     2163           
  Partials      465      465           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@galvana galvana self-requested a review November 29, 2023 22:58
@galvana galvana merged commit c94a3ea into main Dec 5, 2023
40 checks passed
@galvana galvana deleted the CON-gong-mg branch December 5, 2023 18:20
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.

2 participants