-
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(ingestion) slack source v2 - now ingests all user and channels #12795
base: master
Are you sure you want to change the base?
Conversation
…o platform resources
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
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.
Probably would be good to have some tests for this - particularly the user lookup logic / platform resource generation
blob=json.dumps(value.to_obj()).encode("utf-8"), | ||
contentType=SerializedValueContentTypeClass.JSON, | ||
schemaType=SerializedValueSchemaTypeClass.PEGASUS, | ||
schemaRef=schema_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.
I thought we had helpers for this stuff in PlatformResourceInfo
or SerializedResourceValue
- is there a reason we're not using that stuff
yield mcp.as_workunit() | ||
else: | ||
logger.error("Failed to fetch team information") | ||
raise Exception("Failed to fetch team information") |
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 structured error reporting https://acryldata.notion.site/Error-reporting-in-ingestion-5343cc6ea0c84633b38070d1a409c569?pvs=74
) and user_obj.real_name: | ||
mutation_required = True | ||
corpuser_editable_info.displayName = user_obj.real_name | ||
if mutation_required: |
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.
can we make a copy of the original corpuser_editable_info
, and then we know a mutation is required if the two objects aren't equal
platform=DATA_PLATFORM_SLACK_URN, instance=self.id | ||
) | ||
|
||
def update_with_team_info(self, team_info: dict) -> None: |
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.
why wouldn't we pass this stuff in as part of the SlackInstance constructor?
or alternatively, classmethod def from_slack_team_info(team_info: dict) -> SlackInstance
Checklist