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 fields to System PUT payload #4116

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

Kelsey-Ethyca
Copy link
Contributor

@Kelsey-Ethyca Kelsey-Ethyca commented Sep 18, 2023

Code Changes

  • add organization_fides_key, registry_id, meta, fidesctl_meta, and dpa_progress to system api so DB is not overwritten with null values

Steps to Confirm

  • create systems using Data flow scan
  • update a system
  • using the network tab see the System PUT call's payload
  • verify these fields are in the payload organization_fides_key, registry_id, meta, fidesctl_meta, and dpa_progress
  • meta should be populated since the scanner was used "meta": { "kubernetes_namespace": "plc" }

Pre-Merge Checklist

…_progress to system api so DB is not overwritten with null values
@cypress
Copy link

cypress bot commented Sep 19, 2023

Passing run #4218 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 5577968 into 4f33692...
Project: fides Commit: 2b58ce6a66 ℹ️
Status: Passed Duration: 01:41 💡
Started: Sep 19, 2023 1:26 AM Ended: Sep 19, 2023 1:28 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

Copy link
Contributor

@adamsachs adamsachs 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 to me! confirmed running the admin UI against a fidesplus BE so that i had the system scanner in place, using the steps to confirm 👍

@@ -102,6 +102,11 @@ export const transformFormValuesToSystem = (formValues: FormValues): System => {
vendor_id: formValues.vendor_id,
ingress: formValues.ingress,
egress: formValues.egress,
meta: formValues.meta,
fidesctl_meta: formValues.fidesctl_meta,
registry_id: formValues.registry_id,
Copy link
Contributor

@pattisdr pattisdr Sep 19, 2023

Choose a reason for hiding this comment

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

We have a weird thing with the registry_id looks like. I have no idea what this field is for, but validation expects an integer, but sqlalchemy expects a string. If FE is sending an integer here, that will fail at sqlalchemy db level. Maybe we leave out this one.

 Failed to update resource (sqlalchemy.dialects.postgresql.asyncpg.Error) <class 'asyncpg.exceptions.DataError'>: invalid input for query argument $6: 1 (expected str, got int)

Copy link
Contributor

Choose a reason for hiding this comment

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

registries are an extremely old and never-used feature...honestly should just remove it

It was conceived as a way to help organizations organize their fides resources into sub-units. Specifically you could assign systems to a registry to logically "group" them. This use-case has never materialized

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay thanks for the background Thomas!

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.

thanks for addressing @Kelsey-Ethyca!

@Kelsey-Ethyca Kelsey-Ethyca merged commit b770602 into main Sep 19, 2023
10 checks passed
@Kelsey-Ethyca Kelsey-Ethyca deleted the kt-4033-populate-system-fields-for-DB branch September 19, 2023 01:31
Kelsey-Ethyca added a commit that referenced this pull request Sep 19, 2023
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.

4 participants