Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding namespace fides_meta support for BigQuery datasets #5294
Adding namespace fides_meta support for BigQuery datasets #5294
Changes from 4 commits
f0abbc6
8de09fa
d0c51c1
57513f6
31bf67a
1ef7a23
eab846a
e99435c
9e7cf27
78d8fb7
490454e
6ba38af
6b4d051
9b3f115
ddc682b
7bd8cd3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only using this in a limited scope. It'd be nice to use this to validate datasets when we link them to a specific connection config but I'm removing this from scope for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, that's an interesting idea. somewhat related to my comment on the plus PR here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i think this makes sense, though i will say it's a bit surprising to me - since the BQ dataset does need to be associated with a project, in reality. i know we may not need that for DSR processing, but that seems to be a bit of an impl detail, rather than a fact about the dataset itself - and i feel like the dataset definition should try to describe the dataset itself as accurately as possible.
that being said, i realize that requiring this may break backward compatibility, and in general leave things less flexible, so i'm not strongly recommending we change it. i'm good with it as it is, ultimately - just wanted to throw in my 2 cents and see what you think on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I think it'd be better for this to be explicit 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating the init to include an optional
namespace_meta
objectThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we make this support a bit more generic? following the pattern we've taken on the D&D side, it feels like we could support (a less strongly-typed)
namespace_meta
attribute on theSQLQueryConfig
generically, and rely on the implementation/subclass to make use of thenamespace_meta
as it sees fit, i.e. in the datasource-specific way.the fact that you've already typed the
Dataset.fides_meta.namespace
as a genericDict
should support this pattern pretty well.what do you think? maybe it doesn't need to be something we cover now, although i'd kinda like to see it, since i feel like it will only get more cumbersome to implement if we don't update it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is valid, I went ahead and made this change. Let me know what you think of my implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right so what i'm thinking is that instead of "casting" the raw_meta here, we'd cast it within the scope of the
BigQueryQueryConfig
, such that we could initialize a genericnamespace_meta
attribute generically on theSQLConnector
base classThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated it to this
The
SQLLikeQueryConfig
super class validates the meta dict based on thenamespace_meta_schema
defined at theBigQueryQueryConfig