-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat(sdk): add audit_actor and audit_stamp methods to DataHubClient #13676
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
Conversation
- Add audit_actor method to extract actor from JWT tokens with fallback support - Add audit_stamp method to create AuditStampClass with actor and timestamp - Use PyJWT library for secure JWT token parsing - Comprehensive test coverage with parameterized tests using freezegun - Add TODO comment to migrate from DEFAULT_ACTOR_URN to new audit_actor method 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
fallback_timestamp: Optional[datetime] = None, | ||
) -> AuditStampClass: | ||
"""Get an AuditStampClass for auditing purposes. | ||
It uses the actor obtained from the audit_actor method and the current timestamp, unless fallback_timestamp is given. |
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.
use google-style docstrings - this formatting looks a bit odd
@@ -118,3 +129,55 @@ def assertions(self) -> AssertionsClient: # type: ignore[return-value] # Type | |||
"AssertionsClient is not installed, please install it with `pip install acryl-datahub-cloud`" | |||
) | |||
return AssertionsClient(self) | |||
|
|||
def audit_actor( |
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.
how do you anticipate this to actually be used? it's not clear to me because this is a client method, but it would need to be used from classes that don't always have access to a client type
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.
good point!
you are thinking of ingestion pipelines, right?
I could:
- move most of the code to some utils
- keep
audit_actor
andaudit_stamp
in theDataHubClient
... so it is accessible to SDK users - add similar methods to
Pipeline
... so it is accessible to ingestion, even ifDataHub(Graph|Client)
is not available
is there any other place where we use the SDK not having DataHub(Graph|Client)
?
any other proposal?
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.
another option would be to push down this functionality, directly to the AuditStampClass
class AuditStampClass(DictWrapper):
@staticmethod
def from_token(
auth_token: Optional[str] = None,
fallback_actor: Optional[Union[CorpUserUrn, CorpGroupUrn]] = None,
fallback_timestamp: Optional[datetime] = None,
) -> AuditStampClass
WDYT?
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.
from datetime import datetime | ||
from typing import Optional, Union, overload | ||
|
||
import jwt |
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 looks like an additional dep?
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.
That was my guess too and was expecting CI builds to fail because of that... and they did, so yes, dep to be added
Superseded by #13710 |
Summary
audit_actor
method to extract actor from JWT tokens with fallback supportaudit_stamp
method to createAuditStampClass
with actor and timestampKey Features
TODO
Making use of these new methods across the codebase to replace hardcoded actor usage is a follow-up task.
Test Plan
🤖 Generated with Claude Code