-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
feat(sdk): add support for institutional memory links #12770
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
# Internally the aspect is called institutionalMemory, and so much of the code | ||
# uses that name. However, the public-facing API is called "links", since | ||
# that's what we call these in the UI. |
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.
In GraphQL and OpenAPI we use InstitutionalMemory, should we align here with the UI or other APIs? 🤔
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.
Because this sdk is for end users, we should align with the UI
@@ -49,6 +50,8 @@ | |||
|
|||
ActorUrn: TypeAlias = Union[CorpUserUrn, CorpGroupUrn] | |||
|
|||
_DEFAULT_ACTOR_URN = CorpUserUrn("__ingestion").urn() |
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.
not in the scope of this PR but, should this "ingestion actor" configurable?
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.
maybe - we don't show it anywhere in the UI, so it's not really particularly relevant. it's here mainly because a couple fields require it for backwards compat
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.
LGTM
Checklist