-
Notifications
You must be signed in to change notification settings - Fork 670
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 org to identifier protos #4663
Conversation
Signed-off-by: Katrina Rogan <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4663 +/- ##
=======================================
Coverage 58.15% 58.15%
=======================================
Files 626 626
Lines 53786 53786
=======================================
Hits 31277 31277
Misses 20001 20001
Partials 2508 2508
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
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.
Does updating the DatasetID
proto in datacatalog automatically update the cache key? If so this basically invalidates all existing caches unless we write a fallback right?
@@ -129,6 +135,9 @@ message NamedEntityListRequest { | |||
// +optional | |||
string filters = 7; | |||
|
|||
// Optional, org key applied to the resource. | |||
string org = 8; | |||
|
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.
nit - ending space is unnecessary i think
flytepropeller/pkg/controller/nodes/catalog/datacatalog/transformer.go
Outdated
Show resolved
Hide resolved
flytepropeller/pkg/controller/nodes/catalog/datacatalog/transformer.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Katrina Rogan <[email protected]>
@hamersaw we don't actually change how we write the DB keys in datacatalog based on these IDL changes in Create requests, we change the metadata accordingly but not the key and in Get requests the datasetKey is still explicitly constructed using the same identifier fields so this should not nuke datacatalog I believe |
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
PTAL @EngHabu |
a3aa3f0
to
53c218a
Compare
Signed-off-by: Katrina Rogan <[email protected]>
Signed-off-by: Katrina Rogan <[email protected]>
Tracking issue
See RFC: #4588
Why are the changes needed?
This change adds an optional 'org' to Flyte core identifiers
What changes were proposed in this pull request?
How was this patch tested?
Tested on a deployment of flyte with the admin code changes pulled in. Did not observe regressions with running workflows
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link