PROD-2663: specify dataset on fides pull #5260

thingscouldbeworse · 2024-09-05T14:50:59Z

Closes #PROD-2663

Description Of Changes

option for resource and key on push/pull

Code Changes

fides pull now has subcommands for each resource type and those subcommands take a fides_key argument to specify which one
if a manifest file already exists for that resource type the file is rewritten with the server version of the resource. Otherwise a new file is created

Steps to Confirm

All these commands should be valid

Pre-Merge Checklist

vercel · 2024-09-05T14:51:02Z

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

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Sep 12, 2024 7:37pm

cypress · 2024-09-05T15:03:48Z

fides Run #9943

Run Properties: Passed #9943 • ca4603a753 ℹ️: Merge 3dfe8e9dc47210b703d86b7f88caf7afc3767d0a into 82324976d9120e7d139c9657b88e...

Project	`fides`
Branch Review	`refs/pull/5260/merge`
Run status	`Passed #9943`
Run duration	`00m 37s`
Commit	`ca4603a753 ℹ️: Merge 3dfe8e9dc47210b703d86b7f88caf7afc3767d0a into 82324976d9120e7d139c9657b88e...`
Committer	`Kirk Hardy`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

NevilleS · 2024-09-06T18:02:10Z

src/fides/cli/options.py

+def resource_type_option(command: Callable) -> Callable:
+    "Add the resource_type option."
+    command = click.option(
+        "--resource-type",


I really prefer this as an argument (e.g. fides pull dataset {key}) than as an option (--resource-type). It just feels out of place to me as an option, and inconsistent with fides ls [resource type]?

Is there a particular reason you didn't use the argument type?

In my mind (and the default behavior for click) an "argument" like that is a requirement, non optional, whereas an option is optional. It looks like we could override the behavior and write a custom class to make optional click arguments if you feel strongly enough

This is all a consideration because fides push by default pushes everything. We could also split out this functionality into a different command

I'd avoid trying to hack the Click internals at all for this, they are well considered to make parsing a terminal command unambiguously.

That said... can you do this without achieve this by just having the "push" command group have a few subcommands with names like:

name=system

name=dataset

name=data_category

name=''

The empty string would effectively be the default sub-command. I think it might let you do that?

If it feels janky and weird, don't do it and follow the standard way for Click apps 👍

pattisdr

I know there's some in-progress work here, just adding a few comments from experimenting with this -

src/fides/cli/commands/ungrouped.py

pattisdr · 2024-09-10T02:04:52Z

src/fides/cli/commands/ungrouped.py

    config = ctx.obj["CONFIG"]
    taxonomy = _parse.parse(manifests_dir)
+    # if the user has specified a specific fides_key, push only that dataset from taxonomy


I'm just confused about the expected behavior. I guess I thought only the particular dataset would be pushed and not system resources and policy resources

In the ticket it specifies that we should be able to push any resource (if I'm reading that correctly) https://ethyca.atlassian.net/browse/PROD-2663 " argument accepts top level resources via keys. (datasets, systems, categories, uses, subjects) "

I guess I interpreted it as only that resource would be pushed or pulled when the argument was supplied. If that's not expected that's fine

hmm, the argument being the fides_key? That is my intention here, I might be missing something. Does it seem like I'm pushing the wrong thing under the old "default" behavior?

It does limit on the fides_key - I have two datasets, fides_db and fides_db_2, and if I do fides push dataset fides_db_2, just fides_db_2 is pushed.

However, I wasn't expecting the policies, data_categories, and subjects, uses, etc. to be also pushed, but if I'm understanding your intent is still to push all other types and only limit the one if a fides key is supplied.

I am running these commands from the .fides directory that has other resources defined

fidesuser@3b88299a132b:/fides$ fides push dataset fides_db_2 > Loaded config from: /fides/.fides/fides.toml Loading resource manifests from: .fides/ Taxonomy successfully created. Orphan Dataset Warning: The following datasets are not found referenced on a System fides_db_2 ---------- Processing data_category resource(s)... PUSHED 85 data_category resource(s). ---------- Processing policy resource(s)... PUSHED 1 policy resource(s). ---------- Processing organization resource(s)... PUSHED 1 organization resource(s). ---------- Processing dataset resource(s)... PUSHED 1 dataset resource(s). ---------- Processing data_use resource(s)... PUSHED 56 data_use resource(s). ---------- Processing data_subject resource(s)... PUSHED 15 data_subject resource(s). ---------- Processing system resource(s)... PUSHED 3 system resource(s).

src/fides/core/pull.py

CHANGELOG.md

pattisdr · 2024-09-12T00:15:35Z

src/fides/cli/commands/ungrouped.py

    config = ctx.obj["CONFIG"]
    taxonomy = _parse.parse(manifests_dir)
+    # if the user has specified a specific fides_key, push only that dataset from taxonomy


It does limit on the fides_key - I have two datasets, fides_db and fides_db_2, and if I do fides push dataset fides_db_2, just fides_db_2 is pushed.

However, I wasn't expecting the policies, data_categories, and subjects, uses, etc. to be also pushed, but if I'm understanding your intent is still to push all other types and only limit the one if a fides key is supplied.

I am running these commands from the .fides directory that has other resources defined

fidesuser@3b88299a132b:/fides$ fides push dataset fides_db_2 > Loaded config from: /fides/.fides/fides.toml Loading resource manifests from: .fides/ Taxonomy successfully created. Orphan Dataset Warning: The following datasets are not found referenced on a System fides_db_2 ---------- Processing data_category resource(s)... PUSHED 85 data_category resource(s). ---------- Processing policy resource(s)... PUSHED 1 policy resource(s). ---------- Processing organization resource(s)... PUSHED 1 organization resource(s). ---------- Processing dataset resource(s)... PUSHED 1 dataset resource(s). ---------- Processing data_use resource(s)... PUSHED 56 data_use resource(s). ---------- Processing data_subject resource(s)... PUSHED 15 data_subject resource(s). ---------- Processing system resource(s)... PUSHED 3 system resource(s).

pattisdr

pausing here, I want to switch to main and make sure I understand current fides push/fides pull behavior, and compare it to here -

Also there's no additional test coverage included here looks like - our cli tests could be more thorough but there's some tests that could be appended to

pattisdr · 2024-09-12T00:30:48Z

src/fides/core/pull.py

+    """
+    if manifests_dir[-1] == "/":
+        manifests_dir = manifests_dir[:-1]
+    manifest_path = f"{manifests_dir}/{resource_type}.yaml"


So if there's already an existing file, instead of reconciling that resource you write to a separate local file? I'm just asking for clarification on whether these were requirements, not asking that it be changed)

This feels a little confusing with multiple definitions floating around:

Say I have one file, resource.yml

I push it up to the server: fides push

Some changes are made in the UI to dataset_1

I pull down just dataset 1 with your new fides pull dataset dataset_1

It gets added to a new file dataset.yaml

Now I have two different definitions in separate yaml files with the same fides key

I can no longer do a fides push:

sqlalchemy.exc.DBAPIError: (sqlalchemy.dialects.postgresql.asyncpg.Error) <class 'asyncpg.exceptions.CardinalityViolationError'>: ON CONFLICT DO UPDATE command cannot affect row a second time HINT: Ensure that no rows proposed for insertion within the same command have duplicate constrained values

pattisdr · 2024-09-12T00:36:11Z

src/fides/cli/commands/pull.py

+        resource_type="dataset",
+        all_resources_file=None,
+    )
+    echo_green(f"Successfully pulled {fides_key} resource from the server.")


This blanket echo_green can be a little confusing -

Yes and it feels redundant with the other. Removed here: 1901145#diff-aa790a7a4419e75ad08bcd578d54f03be70c59691b10c20c3857fcf5a9bdb783L83

pattisdr

good call to do a more targeted change here.

To capture what I understand is the behavior here:

A fides pull dataset test_dataset command will write the test_dataset definition to a brand new dataset.yaml file even if there is already a dataset definition within the current directory in a separate pre-existing file of a different name.

I think a little more clarity in the docstrings would be useful

pattisdr · 2024-09-12T18:10:59Z

src/fides/core/pull.py

+    resource_type: str,
+) -> None:
+    """
+    Pull a resource from the server by its fides_key and update the local manifest file.


What might help clarify here are talking about the file its going to - you're writing out to a separate file you're likely creating.

cypress · 2024-09-12T19:51:03Z

fides Run #9944

Run Properties: Passed #9944 • b6dee79e00: PROD-2663: specify dataset on fides pull (#5260)

Project	`fides`
Branch Review	`main`
Run status	`Passed #9944`
Run duration	`00m 37s`
Commit	`b6dee79e00: PROD-2663: specify dataset on fides pull (#5260)`
Committer	`Kirk Hardy`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

thingscouldbeworse added 2 commits September 4, 2024 09:31

--fides-key option for datasets on push

260444d

pull can be used to get a specific resource by key

210d550

thingscouldbeworse added 2 commits September 5, 2024 10:52

debug mode thing

5682c8f

work with push

5f7c64c

thingscouldbeworse added 4 commits September 5, 2024 15:11

fix push

b05de23

Merge branch 'main' into PROD-2663_fides-pull-specify-dataset

5b07a7a

try to capture error on CI

5a62bcd

merge and changelog

4127289

NevilleS reviewed Sep 6, 2024

View reviewed changes

pattisdr self-requested a review September 9, 2024 19:56

pattisdr reviewed Sep 10, 2024

View reviewed changes

thingscouldbeworse added 3 commits September 10, 2024 16:11

push individual dataset/system/etc as subcommand

c7d2ff0

pull as group/subcommands as well

3baa8f5

Merge branch 'main' into PROD-2663_fides-pull-specify-dataset

cb587b4

vercel bot deployed to Preview September 11, 2024 11:48 View deployment

thingscouldbeworse added 10 commits September 11, 2024 07:48

linters

3602e59

change default behavior

14287bc

same for pull

f827923

Dawn feedback

828cbe8

get ordering

cb69815

update requirements

ff464c3

add new package to mypy overrides

51e30f5

same for push

3d11cd2

no pass

88e6ab5

Merge branch 'main' into PROD-2663_fides-pull-specify-dataset

9604116

vercel bot deployed to Preview September 11, 2024 19:03 View deployment

thingscouldbeworse added 2 commits September 11, 2024 16:36

de pluralize resource

9bbc00c

Merge branch 'main' into PROD-2663_fides-pull-specify-dataset

3b62dc9

vercel bot deployed to Preview September 11, 2024 20:40 View deployment

revert debug

a5e671f

pattisdr reviewed Sep 12, 2024

View reviewed changes

thingscouldbeworse added 3 commits September 12, 2024 09:26

naming

1b7daac

Merge branch 'main' into PROD-2663_fides-pull-specify-dataset

6d32125

revert push changes

779c704

thingscouldbeworse changed the title ~~PROD-2663: specify dataset on fides push and pull~~ PROD-2663: specify dataset on fides pull Sep 12, 2024

thingscouldbeworse added 3 commits September 12, 2024 10:07

add test for pull

1901145

unused import, test added

6dda9a1

changelog

6c0f546

pattisdr approved these changes Sep 12, 2024

View reviewed changes

docstring update

3dfe8e9

thingscouldbeworse merged commit b6dee79 into main Sep 12, 2024
8 of 9 checks passed

thingscouldbeworse deleted the PROD-2663_fides-pull-specify-dataset branch September 12, 2024 19:34

thingscouldbeworse added a commit that referenced this pull request Sep 12, 2024

PROD-2663: specify dataset on fides pull (#5260)

a1ce570

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROD-2663: specify dataset on fides pull #5260

PROD-2663: specify dataset on fides pull #5260

thingscouldbeworse commented Sep 5, 2024 •

edited

Loading

vercel bot commented Sep 5, 2024 •

edited

Loading

cypress bot commented Sep 5, 2024 •

edited

Loading

NevilleS Sep 6, 2024 •

edited

Loading

thingscouldbeworse Sep 9, 2024

thingscouldbeworse Sep 9, 2024

NevilleS Sep 9, 2024

pattisdr left a comment

pattisdr Sep 10, 2024

thingscouldbeworse Sep 10, 2024

pattisdr Sep 10, 2024

thingscouldbeworse Sep 11, 2024

pattisdr Sep 12, 2024 •

edited

Loading

pattisdr Sep 12, 2024 •

edited

Loading

pattisdr left a comment

pattisdr Sep 12, 2024

pattisdr Sep 12, 2024

thingscouldbeworse Sep 12, 2024

pattisdr left a comment

pattisdr Sep 12, 2024

cypress bot commented Sep 12, 2024

PROD-2663: specify dataset on fides pull #5260

PROD-2663: specify dataset on fides pull #5260

Conversation

thingscouldbeworse commented Sep 5, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Sep 5, 2024 • edited Loading

cypress bot commented Sep 5, 2024 • edited Loading

NevilleS Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

pattisdr Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Sep 12, 2024

thingscouldbeworse commented Sep 5, 2024 •

edited

Loading

vercel bot commented Sep 5, 2024 •

edited

Loading

cypress bot commented Sep 5, 2024 •

edited

Loading

NevilleS Sep 6, 2024 •

edited

Loading

pattisdr Sep 12, 2024 •

edited

Loading

pattisdr Sep 12, 2024 •

edited

Loading